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

[CHANGED] Generate consumer name in Subscribe() when name is not provided #1261

Merged
merged 1 commit into from
May 11, 2023

Conversation

piotrpio
Copy link
Collaborator

@piotrpio piotrpio commented May 11, 2023

This PR changes the way consumer is created in Subscribe() when not using `nats.Durable().

  • Previously, client used CONSUMER.CREATE.<stream_name> subject to create ephemeral consumer, consumer name being generated on the server.
  • Now, consumer name is generated in the client, so that either CONSUMER.CREATE.<stream_name>.<consumer_name> or CONSUMER.CREATE.<stream_name>.<consumer_name>.<filter_subject> is used
  • Ordered consumer uses the new subjects as well

@piotrpio piotrpio marked this pull request as draft May 11, 2023 10:40
@piotrpio piotrpio force-pushed the subscribe-set-consumer-name branch from 60acfe5 to 735636e Compare May 11, 2023 11:51
@coveralls
Copy link

coveralls commented May 11, 2023

Coverage Status

Coverage: 85.209% (+0.1%) from 85.075% when pulling ff32698 on subscribe-set-consumer-name into c3cae07 on main.

@piotrpio piotrpio requested review from Jarema and wallyqs May 11, 2023 12:07
@piotrpio piotrpio marked this pull request as ready for review May 11, 2023 12:07
…ided

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio force-pushed the subscribe-set-consumer-name branch from 735636e to ff32698 Compare May 11, 2023 12:08
@derekcollison
Copy link
Member

Be good to have a Name() option for js.Subscribe() as well, don't think that exists.

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

@piotrpio
Copy link
Collaborator Author

@derekcollison agreed, although I'd call it ConsumerName() to avoid confusion... I'll add it in a separate PR.

@piotrpio piotrpio merged commit d313991 into main May 11, 2023
4 checks passed
@piotrpio piotrpio deleted the subscribe-set-consumer-name branch May 11, 2023 14:56
@derekcollison
Copy link
Member

Thanks!

@piotrpio piotrpio mentioned this pull request May 23, 2023
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.

None yet

4 participants