-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Transaction] Fix individual ack with transaction decrease unAckMessageCounnt #14020
[Transaction] Fix individual ack with transaction decrease unAckMessageCounnt #14020
Conversation
@congbobo184:Thanks for your contribution. For this PR, do we need to update docs? |
@congbobo184:Thanks for providing doc info! |
if (isAcknowledgmentAtBatchIndexLevelEnabled) { | ||
long[] cursorAckSet = getCursorAckSet(position); | ||
if (cursorAckSet != null) { | ||
ackedCount = batchSize - BitSet.valueOf(cursorAckSet).cardinality(); | ||
} else { | ||
ackedCount = batchSize; | ||
} | ||
} else { | ||
ackedCount = batchSize; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a little suggestion. Could we reuse this code block for methods individualAckNormal
and individualAckWithTransaction
?
…ding_ack_reduce_unack_message_count
@@ -249,6 +246,63 @@ public void produceAbortTest() throws Exception { | |||
log.info("finished test partitionAbortTest"); | |||
} | |||
|
|||
@Test(dataProvider="enableBatch") | |||
private void testAckWithTransactionReduceUnAckMessageCount(boolean enableBatch) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should enable batch index ack for the broker and the consumer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransactionTestBase has enable batch index ack
…geCounnt (#14020) link #13383 ## Motivation #13383 has fixed the batch message ack does not decrease the unacked-msg count, but ack with transaction don't fix because decrease unAckMessageCount move to another method. ack with transaction can't decrease unackMessageCount. (cherry picked from commit 1e2ff8a)
…geCounnt (apache#14020) link apache#13383 ## Motivation apache#13383 has fixed the batch message ack does not decrease the unacked-msg count, but ack with transaction don't fix because decrease unAckMessageCount move to another method. ack with transaction can't decrease unackMessageCount.
link #13383
Motivation
#13383 has fixed the batch message ack does not decrease the unacked-msg count, but ack with transaction don't fix
because decrease unAckMessageCount move to another method. ack with transaction can't decrease unackMessageCount.
Modifications
change ack with transaction decrease unackMessageCount to normal ack(ack no transaction).
Verifying this change
Add the tests for it
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)