[Client] Reduce redundant FLOW requests for non-durable multi-topics consumer #11802
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BewareMyPower
added
type/bug
BewareMyPower
requested review from
eolivelli,
sijie,
codelipenghui,
315157973,
aahmed-se and
hangc0276
and removed request for
eolivelli and
sijie
lhotari
approved these changes
Sep 1, 2021
eolivelli
approved these changes
Sep 1, 2021
|
@sijie @codelipenghui @hangc0276 Could you also take a look? |
sijie
approved these changes
Sep 2, 2021
codelipenghui
pushed a commit
that referenced
this pull request
Sep 9, 2021
…11802) ### Motivation #3960 fixed the bug that reader will get stuck if it's reading a partition of a partitioned topic. The fix is using `isDurable` to check whether the consumer is a reader's internal consumer because it used `partitionIndex` to check whether the target topic is a partition while reader's `partitionIndex` is already set. However, for a non-durable multi-topics consumer, `isDurable` is false and each internal consumer will send FLOW request once the connection is established, which is unnecessary because `MultiTopicsConsumerImpl#startReceivingMessages` will send FLOW requests for each internal consumer after all internal consumers are connected. After #4591 introduced `hasParentConsumer` field, the check works for even a reader without the `isDurable` check. ### Modifications - Remove the check for `isDurable` before sending FLOW request and update the related comment. - Add a test for non-durable multi-topics consumer to verify the number of FLOW requests is the topics number, not the twice the topics number. ### Verifying this change - [x] Make sure that the change passes the CI checks. This change added `NonDurableSubscriptionTest#testFlowCountForMultiTopics` and the existing test `ReaderTest#testReadFromPartition` added in #3960 can also pass after this change. This change added tests and can be verified as follows: (cherry picked from commit 1303e7d)
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
…pache#11802) ### Motivation apache#3960 fixed the bug that reader will get stuck if it's reading a partition of a partitioned topic. The fix is using `isDurable` to check whether the consumer is a reader's internal consumer because it used `partitionIndex` to check whether the target topic is a partition while reader's `partitionIndex` is already set. However, for a non-durable multi-topics consumer, `isDurable` is false and each internal consumer will send FLOW request once the connection is established, which is unnecessary because `MultiTopicsConsumerImpl#startReceivingMessages` will send FLOW requests for each internal consumer after all internal consumers are connected. After apache#4591 introduced `hasParentConsumer` field, the check works for even a reader without the `isDurable` check. ### Modifications - Remove the check for `isDurable` before sending FLOW request and update the related comment. - Add a test for non-durable multi-topics consumer to verify the number of FLOW requests is the topics number, not the twice the topics number. ### Verifying this change - [x] Make sure that the change passes the CI checks. This change added `NonDurableSubscriptionTest#testFlowCountForMultiTopics` and the existing test `ReaderTest#testReadFromPartition` added in apache#3960 can also pass after this change. This change added tests and can be verified as follows:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
#3960 fixed the bug that reader will get stuck if it's reading a partition of a partitioned topic. The fix is using
isDurableto check whether the consumer is a reader's internal consumer because it usedpartitionIndexto check whether the target topic is a partition while reader'spartitionIndexis already set. However, for a non-durable multi-topics consumer,isDurableis false and each internal consumer will send FLOW request once the connection is established, which is unnecessary becauseMultiTopicsConsumerImpl#startReceivingMessageswill send FLOW requests for each internal consumer after all internal consumers are connected.After #4591 introduced
hasParentConsumerfield, the check works for even a reader without theisDurablecheck.Modifications
isDurablebefore sending FLOW request and update the related comment.Verifying this change
This change added
NonDurableSubscriptionTest#testFlowCountForMultiTopicsand the existing testReaderTest#testReadFromPartitionadded in #3960 can also pass after this change.This change added tests and can be verified as follows: