-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 the transaction marker doe not deleted as expect. #11126
[Transaction] Fix the transaction marker doe not deleted as expect. #11126
Conversation
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.
Great work,
I left only one comment about shutting down the thread pool
@@ -447,6 +448,13 @@ public void start() throws Exception { | |||
} | |||
} | |||
|
|||
if (pulsar.getConfiguration().isTransactionCoordinatorEnabled()) { | |||
this.transactionBufferDeleteMarkerMonitor = Executors |
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.
can you please "shutdown" this Executor service on BrokerService close ?
I also cannot find the shutdown for these other threadpools: topicPublishRateLimiterMonitor, brokerPublishRateLimiterMonitor, deduplicationSnapshotMonitor
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.
good catch! I will follow your advice.
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.
Looks like the regular task for checking the marker deletion can't fix #11002, if the check task have not triggered yet, we will peek a wrong message right?
I think the right fix is to skip the markers when peeking messages and we don't need a regular checker for the marker deletion, If we have many topics in a broker, the checker will waste more CPU cycles. Or we can say it's an expected behavior(Allow markers that have not been deleted within a period of time)
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St | |||
} | |||
} | |||
|
|||
if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled() | |||
&& this.pendingAckHandle.isTransactionAckPresent()) { |
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.
@codelipenghui #11002 cause by this code, because delete marker judge the pending ack state, if one sub don't use pending ack, it will not delete marker. if we don't start a monitor to check this, how we fix this race condition.
@codelipenghui the peekMessage problem is only a symptom (and it is good to fix it even by skipping messages) |
@eolivelli I have a discussion with @congbobo184 , I think the correct fix is to trigger the marker deletion test properly. The root cause is due to the race condition the marker deletion task has not been triggered due to the previous marker deletion task does not completed, so we need to re-trigger the marker deletion task if there are acks that happen during the last marker deletion task processing. |
So IIUC we are not going to add an additional ThreadPool, |
I think we don't need to add an additional thread pool, we just need to add a check when we complete the last delete marker task. |
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.
great
I left one comment
PTAL
@@ -77,7 +81,7 @@ public void testTransactionMarkerDelete() throws Exception { | |||
managedLedger.openCursor("test"), false); | |||
|
|||
Position position1 = managedLedger.addEntry("test".getBytes()); | |||
managedLedger.addEntry(Markers | |||
Position position2 = managedLedger.addEntry(Markers |
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.
can we simulate a sequence with multiple transactions markers:
- send
- commit
- send
- commit
...
and verify that there is no hole ?
also
- send (with tx)
- commit
- commit
- commit
- send (no tx)
....
@eolivelli add some test like your comment. please review again! thanks. |
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.
looks good to me
@codelipenghui
you left some comments, please take a look again
@congbobo184 Could you please help update the description of the PR? And add more context about how the root cause is and how the PR can fix the issue, this will make others can understand it easily. |
@codelipenghui can you cherry pick to 2.8.1 ? |
…pache#11126) Trigger transaction maker deletes again if there are new delete operations happen during the last delete task processing. (cherry picked from commit 3fcfb22)
…pache#11126) Trigger transaction maker deletes again if there are new delete operations happen during the last delete task processing. (cherry picked from commit 3fcfb22)
…pache#11126) Trigger transaction maker deletes again if there are new delete operations happen during the last delete task processing.
Motivation
fix #11002
implement
Trigger transaction maker deletes again if there are new delete operations happen during the last delete task processing.
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)