You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/28 04:01:06 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #11126: [Transaction] Transaction delete marker monitor.
congbobo184 opened a new pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126
## Motivation
fix https://github.com/apache/pulsar/issues/11002
## implement
start a monitor to delete transaction buffer.
### 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870250827
@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 complete, so we need to re-trigger the marker deletion task if there are acks that happens during the last marker deletion task processing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r659523201
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -447,6 +448,13 @@ public void start() throws Exception {
}
}
+ if (pulsar.getConfiguration().isTransactionCoordinatorEnabled()) {
+ this.transactionBufferDeleteMarkerMonitor = Executors
Review comment:
can you please "shutdown" this Executor service on BrokerService close ?
I also cannot find the shutdown for these other threadpools: topicPublishRateLimiterMonitor, brokerPublishRateLimiterMonitor, deduplicationSnapshotMonitor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui edited a comment on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
codelipenghui edited a comment on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870250827
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r659904127
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St
}
}
- if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled()
- && this.pendingAckHandle.isTransactionAckPresent()) {
Review comment:
@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
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St
}
}
- if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled()
- && this.pendingAckHandle.isTransactionAckPresent()) {
Review comment:
@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 it?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St
}
}
- if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled()
- && this.pendingAckHandle.isTransactionAckPresent()) {
Review comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11126: [Transaction] Fix the transaction marker doe not deleted as expect.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-881342614
@codelipenghui can you cherry pick to 2.8.1 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870255104
@codelipenghui
> the correct fix is to trigger the marker deletion test properly
makes sense
So IIUC we are not going to add an additional ThreadPool,
@congbobo184 regarding the shutdown of the executors I will be happy to send the patch
it is better to address the problem in a separate patch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870250827
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r659904127
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St
}
}
- if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled()
- && this.pendingAckHandle.isTransactionAckPresent()) {
Review comment:
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r659904127
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St
}
}
- if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled()
- && this.pendingAckHandle.isTransactionAckPresent()) {
Review comment:
@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 it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870416270
@eolivelli
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-872929910
@eolivelli add some test like your comment. please review again! thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870246303
@codelipenghui the peekMessage problem is only a symptom (and it is good to fix it even by skipping messages)
The real problem IMHO is that the list of individually deleted messages will grow without bounds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870246303
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r662195865
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/service/TransactionMarkerDeleteTest.java
##########
@@ -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
Review comment:
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)
....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-880657017
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r659904127
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -390,16 +391,7 @@ public void acknowledgeMessage(List<Position> positions, AckType ackType, Map<St
}
}
- if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled()
- && this.pendingAckHandle.isTransactionAckPresent()) {
Review comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui edited a comment on pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
codelipenghui edited a comment on pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#issuecomment-870250827
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #11126: [Transaction] Transaction delete marker monitor.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126#discussion_r659524870
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -447,6 +448,13 @@ public void start() throws Exception {
}
}
+ if (pulsar.getConfiguration().isTransactionCoordinatorEnabled()) {
+ this.transactionBufferDeleteMarkerMonitor = Executors
Review comment:
good catch! I will follow your advice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #11126: [Transaction] Fix the transaction marker doe not deleted as expect.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #11126:
URL: https://github.com/apache/pulsar/pull/11126
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org