Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMPROVED] Subscribe returns error if consumer config does not match #803

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

kozlovic
Copy link
Member

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

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>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@coveralls
Copy link

coveralls commented Aug 23, 2021

Coverage Status

Coverage increased (+0.3%) to 87.524% when pulling e2c6dff on fix_796 into fc13b0b on master.

Signed-off-by: Waldemar Quevedo <wally@synadia.com>
Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Return error if actual consumer configuration is different
4 participants