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 bug of Key_Shared Ordering #4372

Merged
merged 6 commits into from
May 28, 2019

Conversation

codelipenghui
Copy link
Contributor

Motivation

Fix bug of Key_Shared ordering and add check for order by key, key distribution, message redelivery in Key_Shared subscription mode.

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

run Integration Tests
run Java8 Tests

@sijie sijie added area/client type/bug The PR fixed a bug or issue reported a bug labels May 26, 2019
@sijie sijie added this to the 2.4.0 milestone May 26, 2019
@sijie
Copy link
Member

sijie commented May 26, 2019

run integration tests

}

protected void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
int start = 0;
int entriesToDispatch = entries.size();
long totalMessagesSent = 0;
long totalBytesSent = 0;
AtomicInteger messagesToConsumer = new AtomicInteger(entries.size());
Copy link
Member

Choose a reason for hiding this comment

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

why do we need change this file? method sendMessagesToConsumers got override in PersistentStickyKeyDispatcherMultipleConsumers right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change not to solve the problem of Key_Shared subscription, i initial think this will reduce the reading frequency of ledger in shared subscription. This will wait for the message to be written to the consumers and then read again.

Copy link
Member

@jiazhai jiazhai May 27, 2019

Choose a reason for hiding this comment

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

I would suggest remove this change. since we have not meet this related issue in Shared mode, reading frequency of ledger is more controlled by other setting.

This is mainly for message ordering, and ordering is not need for shared mode.

Copy link
Contributor Author

@codelipenghui codelipenghui May 27, 2019

Choose a reason for hiding this comment

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

Agree, i'm already remove this change, PTAL.

@jiazhai
Copy link
Member

jiazhai commented May 26, 2019

run java8 tests
run integration tests

2 similar comments
@codelipenghui
Copy link
Contributor Author

run java8 tests
run integration tests

@jiazhai
Copy link
Member

jiazhai commented May 27, 2019

run java8 tests
run integration tests

@jiazhai
Copy link
Member

jiazhai commented May 27, 2019

run integration tests

@codelipenghui
Copy link
Contributor Author

run Integration Tests

1 similar comment
@jiazhai
Copy link
Member

jiazhai commented May 28, 2019

run Integration Tests

@codelipenghui
Copy link
Contributor Author

@jiazhai According to what we discussed, we need an ordered LongPairSet to support ordered guarantees of message re-delivery in key_shared subscription, added a task to track this feature #4077 , i will work with a new PR.

@jiazhai jiazhai merged commit 7a21094 into apache:master May 28, 2019
@codelipenghui codelipenghui deleted the fix_key_shared_ordering 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
area/client 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.

None yet

3 participants