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

Key_Shared subscription on non-persistent topic #4462

Merged
merged 2 commits into from
Jun 6, 2019

Conversation

codelipenghui
Copy link
Contributor

Motivation

Support Key_Shared subscription on non-persistent topic

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (o)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: ( no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@codelipenghui
Copy link
Contributor Author

relate to #4077

@codelipenghui
Copy link
Contributor Author

/cc @wolfstudy please help review the changes

@jiazhai
Copy link
Member

jiazhai commented Jun 4, 2019

run cpp tests

@jiazhai jiazhai added this to the 2.4.0 milestone Jun 4, 2019
@codelipenghui
Copy link
Contributor Author

run java8 tests

@codelipenghui codelipenghui force-pushed the key_shared_non_persistent branch from 8a6fe0e to 4dc111a Compare June 5, 2019 02:16
@codelipenghui codelipenghui force-pushed the key_shared_non_persistent branch from 4dc111a to 60c2bf4 Compare June 5, 2019 02:17
@wolfstudy
Copy link
Member

LGTM +1

run java8 tests

int consumer1ExpectMessages = 0;
int consumer2ExpectMessages = 0;
int consumer3ExpectMessages = 0;

Copy link
Member

Choose a reason for hiding this comment

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

In the test case, the code of newConsumer() and newProducer() is repeated multiple times, whether consider encapsulation as a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion, i'm already and separate method newConsumer and newProducer to create consumer and producer, PTAL.

Copy link
Member

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants