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

Fix remove pending acks in Key_Shared subscription #4407

Merged
merged 4 commits into from
May 31, 2019

Conversation

codelipenghui
Copy link
Contributor

Motivation

Fix remove pending acks in Key_Shared subscription and some other stats issue of Key_Shared subscription

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • 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

@wolfstudy Thank you very much for discovering this problem in time. Please review the changes.

@codelipenghui
Copy link
Contributor Author

run Java8 Tests

@wolfstudy
Copy link
Member

LGTM +1

@@ -334,7 +334,7 @@ void messageAcked(CommandAck ack) {
return;
}

if (subType == SubType.Shared) {
if (subType == SubType.Shared || subType == SubType.Key_Shared) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is being checked multiple times, we could have a static method to check for this condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly this is being checked multiple times, i'm already add static method named Subscription.isIndividualAckMode(), PTAL

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label May 31, 2019
@merlimat merlimat added this to the 2.4.0 milestone May 31, 2019
@merlimat merlimat merged commit 4464dbb into apache:master May 31, 2019
@codelipenghui codelipenghui deleted the fix_remove_pending_acks branch May 19, 2021 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants