Skip to content

[Issue 8346][Admin API] Validate retention policy - #8358

Merged
codelipenghui merged 2 commits into
apache:masterfrom
lhotari:lh-validate-retention-policies
Oct 26, 2020
Merged

[Issue 8346][Admin API] Validate retention policy#8358
codelipenghui merged 2 commits into
apache:masterfrom
lhotari:lh-validate-retention-policies

Conversation

@lhotari

@lhotari lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member

Fixes #8346

Motivation

See #8346, #8345

Modifications

Prevent setting invalid retention policy where either size or time limit is set to zero while the other limit has a non-zero value.
The reason for this is that setting either size or time limit to zero will effectively disable the retention policy.
It is confusing for the end-user if it's possible to set a value for the other limit while it's ignored when the other limit has
the value of zero. This might lead to the incorrect assumption that the value of 0 has the meaning that the limit isn't effective.
The validation will provide the Admin API end user a useful error message if the validation fails.

@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

2 similar comments
@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@jiazhai
jiazhai requested a review from codelipenghui October 23, 2020 12:57
@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

since master branch is broken atm, I cherry picked the commit from #8361 to fix the broken state of CI. That PR should be merged before this one.

@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

Comment thread pom.xml Outdated
@sijie sijie added this to the 2.7.0 milestone Oct 23, 2020
@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 23, 2020

Copy link
Copy Markdown
Member Author

I'm running an experiment to see if reverting 647d3c2 fixes the grpc / protobuf compatibility issues.

Fixes apache#8346

- prevent setting invalid retention policy where either size or time
  limit is set to zero while the other limit has a non-zero value.
  - the reason for this is that setting either size or time limit to
    zero will effectively disable the retention policy.
    - it is confusing for the end-user if it's possible to set a value
      for the other limit while it's ignored when the other limit has
      the value of zero
@lhotari
lhotari force-pushed the lh-validate-retention-policies branch from 30aa4c8 to 12a463a Compare October 23, 2020 19:50
@lhotari
lhotari marked this pull request as ready for review October 23, 2020 20:17
@lhotari
lhotari marked this pull request as draft October 23, 2020 20:42

@codelipenghui codelipenghui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@codelipenghui

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

@lhotari
lhotari marked this pull request as ready for review October 24, 2020 18:30
@lhotari

lhotari commented Oct 24, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

6 similar comments
@lhotari

lhotari commented Oct 24, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 24, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 25, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 25, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 25, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@lhotari

lhotari commented Oct 25, 2020

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@codelipenghui
codelipenghui merged commit db5320d into apache:master Oct 26, 2020
@lhotari
lhotari deleted the lh-validate-retention-policies branch October 26, 2020 16:54
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Nov 13, 2020
Fixes apache#8346

### Motivation

See apache#8346, apache#8345

### Modifications

Prevent setting invalid retention policy where either size or time limit is set to zero while the other limit has a non-zero value.
The reason for this is that setting either size or time limit to zero will effectively disable the retention policy.
It is confusing for the end-user if it's possible to set a value for the other limit while it's ignored when the other limit has
the value of zero. This might lead to the incorrect assumption that the value of 0 has the meaning that the limit isn't effective.
The validation will provide the Admin API end user a useful error message if the validation fails.
flowchartsman pushed a commit to flowchartsman/pulsar that referenced this pull request Nov 17, 2020
Fixes apache#8346

### Motivation

See apache#8346, apache#8345

### Modifications

Prevent setting invalid retention policy where either size or time limit is set to zero while the other limit has a non-zero value.
The reason for this is that setting either size or time limit to zero will effectively disable the retention policy.
It is confusing for the end-user if it's possible to set a value for the other limit while it's ignored when the other limit has
the value of zero. This might lead to the incorrect assumption that the value of 0 has the meaning that the limit isn't effective.
The validation will provide the Admin API end user a useful error message if the validation fails.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Admin API] Prevent setting an invalid Retention Policy

3 participants