[Transaction]Transaction pendingack server implement patch #8426
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
added 2 commits
November 2, 2020 17:42
|
/pulsarbot run-failure-checks |
1 similar comment
|
/pulsarbot run-failure-checks |
50 tasks
Comment on lines
61
to
63
| if (currentPosition == null || otherPosition ==null) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
It's better to throw an exception here, if return -1 it means the currentPosition is lower than otherPosition
Comment on lines
68
to
70
| if (otherPosition.getAckSet() == null) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Why otherPosition's ackSet is null then return the result?
Comment on lines
72
to
91
| BitSetRecyclable otherAckSet; | ||
| if (currentPosition.getAckSet() == null) { | ||
| if (otherPosition.getAckSet() != null) { | ||
| otherAckSet = BitSetRecyclable.valueOf(otherPosition.getAckSet()); | ||
| if (otherAckSet.isEmpty()) { | ||
| otherAckSet.recycle(); | ||
| return result; | ||
| } else { | ||
| otherAckSet.recycle(); | ||
| return 1; | ||
| } | ||
|
|
||
| } | ||
| return result; | ||
| } | ||
| otherAckSet = BitSetRecyclable.valueOf(otherPosition.getAckSet()); | ||
| BitSetRecyclable thisAckSet = BitSetRecyclable.valueOf(currentPosition.getAckSet()); | ||
| result = thisAckSet.nextSetBit(0) - otherAckSet.nextSetBit(0); | ||
| otherAckSet.recycle(); | ||
| thisAckSet.recycle(); |
There was a problem hiding this comment.
I think we can use an empty ackset to simplify these lines.
Comment on lines
460
to
461
| return FutureUtil.failedFuture( | ||
| new TransactionConflictException(error)); |
There was a problem hiding this comment.
If this for normal ack, shall we need to return TransactionConflictException?
|
|
||
|
|
||
| //this method is for individual ack carry the transaction | ||
| private CompletableFuture<Void> individualAckWithTransaction(CommandAck ack) { |
There was a problem hiding this comment.
Looks there is much duplicate code with the individualAckNormal, is it possible to refine it?
| @@ -131,12 +132,13 @@ | |||
| private static final double avgPercent = 0.9; | |||
| private boolean preciseDispatcherFlowControl; | |||
| private PositionImpl readPositionWhenJoining; | |||
| private final boolean isTransactionEnabled; | |||
There was a problem hiding this comment.
Move the isTransactionEnabled to the topic or subscription? We cannot treat consumers differently in transaction.
gaoran10
reviewed
Nov 10, 2020
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
codelipenghui
approved these changes
Nov 10, 2020
…l/ManagedCursorImpl.java Co-authored-by: ran <gaoran_10@126.com>
…tion_pendingack_server_implement
…ck_server_implement' into congbobo184_transaction_pendingack_server_implement
flowchartsman
pushed a commit
to flowchartsman/pulsar
that referenced
this pull request
Nov 17, 2020
Fix apache#7981 this PR patch apache#8256 ## Motivation - we need to support transaction pending-ack stat, and we need to support bacth ack. ## implement - client ack with transaction will carry only this ack bit set 1 1 1 1 1 0 1 1，the 6 point is this transaction ack bit set point. - commit or abort will recover the bit set point to 1 ### 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)
This was referenced Nov 21, 2020
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.
Fix #7981
this PR patch #8256
Motivation
implement
client ack with transaction will carry only this ack bit set 1 1 1 1 1 0 1 1，the 6 point is this transaction ack bit set point.
we will find the batch size from consumer pendingAcks, when we don't find it ,the ack will fail.
the normal individual ack will sync the bitch size to pending ack handle.
we will remove the position from consumer pending acks after normal ack sync finish then check if batch position in pending ack handle all acked.
when the position acked finish, we will check it is less than mark delete position then we will clear the position from pending ack handle.
abort the transaction, we will recover the transaction acked batch point to 1.
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)