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.

Sign up for GitHub

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

Jump to bottom

Fix consumer stuck issue due to reuse entry wrapper. #10824

Merged
merged 4 commits into from
Jun 4, 2021
Merged

Fix consumer stuck issue due to reuse entry wrapper. #10824

merged 4 commits into from
Jun 4, 2021

Conversation

codelipenghui
Copy link
Contributor

Fixes #10813
The issue is introduced by #7266, it only affects the master branch.

Motivation

  1. Add wrapperOffset to make sure get the correct batch size from the metadata
  2. Fix the issue that using (batch count / avgBatchSizePerMsg) to calculate messages for the consumer
    it should be (messages / avgBatchSizePerMsg)

Verifying this change

 * The test case is to simulate dispatch batches with different batch size to the consumer.
 * 1. The consumer has 1000 available permits
 * 2. The producer send batches with different batch size
 *
 * According the batch average size dispatching, the broker will dispatch all the batches to the consumer

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
Fix consumer stuck issue due to reuse entry wrapper.
f313f61 
1. Add wrapperOffset to make sure get the correct batch size from the metadata
2. Fix the issue that using (batch count / avgBatchSizePerMsg) to calculate messages for the consumer
   it should be (messages / avgBatchSizePerMsg)
@codelipenghui
Fix comment of the test
785b33c
@codelipenghui
Fix comment of the test
2e53355
@codelipenghui codelipenghui self-assigned this Jun 4, 2021
@codelipenghui codelipenghui added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Jun 4, 2021
@codelipenghui codelipenghui added this to the 2.8.0 milestone Jun 4, 2021
lhotari
lhotari approved these changes Jun 4, 2021
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM. I added 2 code review comments which are more like questions where I'm trying to understand the overall code base around message dispatching. Good work Penghui!

@eolivelli
Copy link
Contributor

The change makes sense to me.
Let me test it locally and see if my case is solved.

Thank you very much !

@lhotari
Copy link
Member

lhotari commented Jun 4, 2021

I have tested the fix. The situation looks better, but all messages don't get delivered from the backlog to the consumer.

congbobo184
congbobo184 approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

LGTM！

@eolivelli
Copy link
Contributor

The fix makes sense to me.
but unfortunately but test case is still failing

eolivelli
eolivelli reviewed Jun 4, 2021
View reviewed changes
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/DispatchAccordingPermitsTests.java
* According the batch average size dispatching, the broker will dispatch all the batches to the consumer
*/
@Test
public void testFlowPermitsWithMultiBatchesDispatch() throws PulsarAdminException, PulsarClientException {
Copy link
Contributor

Choose a reason for hiding this comment

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

the main difference between my test and this test is that here we do not have a partitioned topic.

a good follow up work is to add a test for partitioned topics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eolivelli I have merged the PR, will try to investigate the problem with the partitioned topic. I have test 100 partitions on the standalone, seem not able to reproduce the issue. Will try to deploy to a real cluster for testing. Will try to write a UT for partitioned case if it can be reproduced

@eolivelli
Copy link
Contributor

let's merge the patch as soon as CI is green

@codelipenghui codelipenghui merged commit 4f23767 into apache:master Jun 4, 2021
@codelipenghui codelipenghui deleted the penghui/fix-10813 branch June 4, 2021 10:14
@eolivelli
Copy link
Contributor

@codelipenghui I would like to add that with this fix my test arrived to 99.85% !
it is a good improvement BTW

thanks !

@eolivelli eolivelli mentioned this pull request Jun 4, 2021
Closed
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2021
@codelipenghui @eolivelli
Fix consumer stuck issue due to reuse entry wrapper. (apache#10824)
f508ff1 
Fixes apache#10813
The issue is introduced by apache#7266, it only affects the master branch.

### Motivation

1. Add wrapperOffset to make sure get the correct batch size from the metadata
2. Fix the issue that using (batch count / avgBatchSizePerMsg) to calculate messages for the consumer
   it should be (messages / avgBatchSizePerMsg)

### Verifying this change

     * The test case is to simulate dispatch batches with different batch size to the consumer.
     * 1. The consumer has 1000 available permits
     * 2. The producer send batches with different batch size
     *
     * According the batch average size dispatching, the broker will dispatch all the batches to the consumer

(cherry picked from commit 4f23767)
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
@codelipenghui @yangl
Fix consumer stuck issue due to reuse entry wrapper. (apache#10824)
eed64a4 
Fixes apache#10813
The issue is introduced by apache#7266, it only affects the master branch.

### Motivation

1. Add wrapperOffset to make sure get the correct batch size from the metadata
2. Fix the issue that using (batch count / avgBatchSizePerMsg) to calculate messages for the consumer
   it should be (messages / avgBatchSizePerMsg)

### Verifying this change

     * The test case is to simulate dispatch batches with different batch size to the consumer.
     * 1. The consumer has 1000 available permits
     * 2. The producer send batches with different batch size
     *
     * According the batch average size dispatching, the broker will dispatch all the batches to the consumer
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@codelipenghui @ciaocloud
Fix consumer stuck issue due to reuse entry wrapper. (apache#10824)
6100fa5 
Fixes apache#10813
The issue is introduced by apache#7266, it only affects the master branch.

### Motivation

1. Add wrapperOffset to make sure get the correct batch size from the metadata
2. Fix the issue that using (batch count / avgBatchSizePerMsg) to calculate messages for the consumer
   it should be (messages / avgBatchSizePerMsg)

### Verifying this change

     * The test case is to simulate dispatch batches with different batch size to the consumer.
     * 1. The consumer has 1000 available permits
     * 2. The producer send batches with different batch size
     *
     * According the batch average size dispatching, the broker will dispatch all the batches to the consumer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@merlimat merlimat merlimat approved these changes

@lhotari lhotari lhotari approved these changes

@congbobo184 congbobo184 congbobo184 approved these changes

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@jiazhai jiazhai Awaiting requested review from jiazhai

@sijie sijie Awaiting requested review from sijie

@zymap zymap Awaiting requested review from zymap

Assignees

@codelipenghui codelipenghui

Labels
release/blocker Indicate the PR or issue that should block the release until it gets resolved
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

Consumer stuck while reading from partitioned topic using Shared subscription
5 participants
@codelipenghui @eolivelli @lhotari @merlimat @congbobo184