[IMPROVED] Subscribe returns error if consumer config does not match #803
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
This is done to alert the user that consumer config is not changed as one would expect. This change is more tricky than expected. We can't simply compare user's consumer config with what is returned from the server. For once, there are configurations that may not be set by the user that were set when the consumer was created, that the lib should not fail. Let's say the consumer is created with a description, why fail the user if they don't pass the Description() option at all? The Description() option was actually missing, but this is a good example. But same could be say with for instance "optional start sequence". This may be something that is set when the consumer is first created (possibly outside of the app with CLI), but when user calls js.Subscribe(), that option should not have to be set to the value that was used when creating the consumer. I had to add "not set" values for replay and deliver policy, similar to ack policy. I added an extensive test that checks proper error when trying to make configuration changes, but also that if the options are not set, then the lib does not fail the subscribe call. Resolves #796 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Signed-off-by: Waldemar Quevedo <wally@synadia.com>
wallyqs
reviewed
Aug 24, 2021
10 tasks
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.
This is done to alert the user that consumer config is not changed
as one would expect.
This change is more tricky than expected. We can't simply compare
user's consumer config with what is returned from the server.
For once, there are configurations that may not be set by the
user that were set when the consumer was created, that the lib
should not fail. Let's say the consumer is created with a description,
why fail the user if they don't pass the Description() option at all?
The Description() option was actually missing, but this is a good
example. But same could be say with for instance "optional start
sequence". This may be something that is set when the consumer
is first created (possibly outside of the app with CLI), but when
user calls js.Subscribe(), that option should not have to be
set to the value that was used when creating the consumer.
I had to add "not set" values for replay and deliver policy, similar
to ack policy.
I added an extensive test that checks proper error when trying
to make configuration changes, but also that if the options are
not set, then the lib does not fail the subscribe call.
Resolves #796
Signed-off-by: Ivan Kozlovic ivan@synadia.com