Remove validation when adding or updating consumer #923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
scottf
commented
Jun 13, 2023
•
scottf
commented
- Do not require name or durable when adding or updating a consumer.
- Do not reject when inactive threshold is set and a durable is set.
scottf
commented
Jun 13, 2023
| @@ -654,12 +654,19 @@ public void testAddDeleteConsumer() throws Exception { | |||
| assertTrue(iae.getMessage().contains("Stream cannot be null or empty")); | |||
| iae = assertThrows(IllegalArgumentException.class, () -> jsm.addOrUpdateConsumer(STREAM, null)); | |||
| assertTrue(iae.getMessage().contains("Config cannot be null")); | |||
| iae = assertThrows(IllegalArgumentException.class, () -> jsm.addOrUpdateConsumer(STREAM, cc)); | |||
| assertTrue(iae.getMessage().contains("Durable cannot be null")); | |||
There was a problem hiding this comment.
No longer validating
scottf
commented
Jun 13, 2023
|
|
||
| // durable and name can both be null | ||
| ConsumerInfo ci = jsm.addOrUpdateConsumer(STREAM, cc); | ||
| assertNotNull(ci.getName()); |
There was a problem hiding this comment.
test that the server gives a name when one is not supplied
scottf
commented
Jun 13, 2023
| iae = assertThrows(IllegalArgumentException.class, () -> jsm.addOrUpdateConsumer(STREAM, cc2)); | ||
| assertTrue(iae.getMessage().contains("Inactive Threshold")); | ||
| ci = jsm.addOrUpdateConsumer(STREAM, cc2); | ||
| assertEquals(10000, ci.getConsumerConfiguration().getInactiveThreshold().toMillis()); |
There was a problem hiding this comment.
test that an inactiveThreshold is allowed when a durable name is supplied
scottf
commented
Jun 13, 2023
| @@ -125,11 +124,6 @@ public PurgeResponse purgeStream(String streamName, PurgeOptions options) throws | |||
| public ConsumerInfo addOrUpdateConsumer(String streamName, ConsumerConfiguration config) throws IOException, JetStreamApiException { | |||
| validateStreamName(streamName, true); | |||
| validateNotNull(config, "Config"); | |||
| validateNotNull(config.getDurable(), "Durable"); // durable name is required when creating consumers | |||
There was a problem hiding this comment.
durable not required
scottf
commented
Jun 13, 2023
| Duration d = config.getInactiveThreshold(); | ||
| if (d != null && !d.isZero()) { | ||
| throw new IllegalArgumentException("Durable consumers cannot have an Inactive Threshold."); | ||
| } |
There was a problem hiding this comment.
inactiveThreshold allowed even when there is a durable
12 tasks
piotrpio
reviewed
Jun 14, 2023
| @@ -106,13 +113,33 @@ public final synchronized String next() { | |||
| // copy in the seq in base36. | |||
There was a problem hiding this comment.
Should say base62 I think.
Suggested change
| // copy in the seq in base36. | |
| // copy in the seq in base62. |
There was a problem hiding this comment.
heh, that was a copy from the method I based this on. Fixing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.