You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "315157973 (via GitHub)" <gi...@apache.org> on 2023/06/26 13:32:10 UTC
[GitHub] [pulsar] 315157973 opened a new pull request, #20649: [fix][broker] Avoid producer latency rise
315157973 opened a new pull request, #20649:
URL: https://github.com/apache/pulsar/pull/20649
### Motivation
The executor in `ManagedLedgerImpl` uses the `MainWorkerPool` of bookKeeper, and both produce and consume use this thread pool.
When there are a large number of partitions in the cluster and the cluster is busy, trim ledger will cause the produce latency to rise from hundreds of milliseconds to seconds. We found that a large number of tasks are stuck in the queue of the thread pool
Trim ledger is a thread-safe operation and has no latency requirements, so we'd better separate it from `MainWorkerPool` of bookKeeper
### Modifications
Separate it from `MainWorkerPool` of bookKeeper
### Verifying this change
### Documentation
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
### Matching PR in forked repository
--
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 #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1617455183
@315157973 Do you have a flame graph or profiler result for the issue? If I understand correctly, the problem is the Trim operation exhausted the BookKeeper main thread pool or exhausted the CPU resources of the pod/node? If it exhausted the CPU resources of the pod/node, it looks like the only way can resolve the issue is to make the Trim operation works more efficiently.
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> This lock is segmented and does not block each producer for a long time.
Make sense
--
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] 315157973 commented on pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1617602645
> @315157973 Do you have a flame graph or profiler result for the issue? If I understand correctly, the problem is the Trim operation exhausted the BookKeeper main thread pool or exhausted the CPU resources of the pod/node? If it exhausted the CPU resources of the pod/node, it looks like the only way can resolve the issue is to make the Trim operation works more efficiently.
The CPU is not full. Each trim operation is time-consuming, because there are locks and other operations that will wait. Assuming that a task takes 0.1ms, 10,000 partitions will take 1s, which will slow down the tasks queued later
--
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] hangc0276 commented on pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1618065043
> > @315157973 Do you have a flame graph or profiler result for the issue? If I understand correctly, the problem is the Trim operation exhausted the BookKeeper main thread pool or exhausted the CPU resources of the pod/node? If it exhausted the CPU resources of the pod/node, it looks like the only way can resolve the issue is to make the Trim operation works more efficiently.
>
> The CPU is not full. Each trim operation is time-consuming, because there are locks and other operations that will wait. Assuming that a task takes 0.1ms, 10,000 partitions will take 1s, which will slow down the tasks queued later
@315157973 Could you share the lock profile flame graph before and after this change? +1 for yubiao's concern about the lock
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250532829
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
@315157973
Now the tasks `publish` and `trim ledgers` all run in the thread `executor`( it is a single thread pool, and the tasks of same topic will be executed in the same thread<sup>[L-1]</sup> ), right?<sup>[Q-1]</sup>
---
if yes<sup>[Q-1]</sup>, the task `trim ledgers` blocks the task `publish`, which this PR tries to fix, right?<sup>[Q-2]</sup>
---
if yes<sup>[Q-2]</sup>, but both tasks `trim ledgers`<sup>[L-2]</sup> and `internalAsyncAddEntry`<sup>[L-3]</sup> will try to acquire the same lock `synchronized(ml)`, so the task `trim ledgers` is still blocking the task `publish` even if the patch is merged, right?[Q-3]</sup>
---
**[L-1]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L348
**[L-2]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2608
**[L-3]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L798
--
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 #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1619877645
I'm trying to reproduce the issue...
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250532829
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
@315157973
Now the tasks `publish` and `trim ledgers` all run in the thread `executor`( it is a single thread pool, and the tasks of same topic will be executed in the same thread<sup>[L-1]</sup> ), right?<sup>[Q-1]</sup>
---
if yes<sup>[Q-1]</sup>, the task `trim ledgers` blocks the task `publish`, which this PR tries to fix, right?<sup>[Q-2]</sup>
---
if yes<sup>[Q-2]</sup>, But both tasks `trim ledgers`<sup>[L-2]</sup> and `internalAsyncAddEntry`<sup>[L-3]</sup> will try to acquire the lock `synchronized(ml)`, so the task `trim ledgers` is still blocking the task `publish` even if the patch is merged, right?[Q-3]</sup>
---
**[L-1]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L348
**[L-2]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2608
**[L-3]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L798
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250492170
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
Both tasks `trim ledgers` and `internalAsyncAddEntry` will try to acquire the lock `synchronized(ml)`, If the task `trim ledgers` running in the thread `executor`, it avoids lock contention.
Then if we make the task `trim ledgers` run in another thread, the publish task will also wait for the lock `synchronized(ml)`. Maybe we need improve this scenario first
--
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] 315157973 commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250541569
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
This lock is segmented and does not block each producer for a long time.
But if they are in the same thread pool, the thread pool will be occupied for a long time
--
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 #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1624828400
I have tried to reproduce the issue in the last two days. It looks like the killer of the publish latency when the broker trimming many Ledgers is the Zookeeper operation latency because all the pending add entry requests need to wait for the new Ledger creation. I don't see the trim operation exhausting the CPU resource or the contention introduced by the trim operation.
#### Test Steps
The test is running or 3 brokers and 3 bookies, 10 CPU cores assigned to the broker, and 6 cores assigned to the bookie.
I have changed the max ledger rollover time to 2 min to make sure each topic will have enough ledgers.
1. Start a test to write and read messages on 30k topics (Non-partitioned topic)
2. Set the data retention for 2 hours
3. Waiting for 2 hours to make sure we have many ledgers will be trimmed
4. Set the data retention to 2 mins after 2 hours
5. Check the dashboard, the publish latency has been impacted.
The P99 publish latency goes up to 15s during the Ledger trimming.
The Zookeeper update latency also goes up to 15s during the Leger trimming.
For each bookie, there are almost 1 million Ledgers have been deleted.
No obvious broker CPU spike
And the flame graph is made during the ledger trimming.
[trim_cpu_0707.html.txt](https://github.com/apache/pulsar/files/11975299/trim_cpu_0707.html.txt)
[trim_lock_0707.html.txt](https://github.com/apache/pulsar/files/11975300/trim_lock_0707.html.txt)
#### Conclusion
I doubt this PR can resolve the publish latency spike issue during the ledger trimming. From the above test, I don't see the bottleneck is from contention or CPU exhaustion. Maybe if you have a very good performance zookeeper, the zookeeper will not be the bottleneck. Then you can figure out other bottlenecks. But the zookeeper reached 8.5k ops per second. I don't think zookeeper is designed for such high-performance requirements
Another solution is to introduce a throttling mechanism for the trim operation. So that we can prevent a huge impact on the cluster when trimming ledgers. It's not too big a problem for users to trim the ledgers later but it can make the trim operation smoothly.
--
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] gaoran10 commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250505265
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
Maybe we can resolve this busy thread problem first, because the synchronized symbol only affects one ManagedLedger object, but one bk main executor thread may handle a lot of trim tasks for different ManagedLedger objects.
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> This lock is segmented and does not block each producer for a long time.
Make sense. The task `trim ledgers` has more than one sub-tasks, only one sub-task will take the lock
--
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
Re: [PR] [improve][broker] Avoid producer latency rise during internal ledger trimming operations [pulsar]
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 closed pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
URL: https://github.com/apache/pulsar/pull/20649
--
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] 315157973 commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1251443345
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
Users with fewer partitions submit fewer tasks to the thread pool
--
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 diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1242212221
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,11 +2451,11 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimLedgers(isTruncate, promise));
+ scheduledExecutor.execute(() -> internalTrimLedgers(isTruncate, promise));
}
private void scheduleDeferredTrimming(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
probably ew can also simplify this method `scheduleDeferredTrimming` and do not call scheduledExecutor twice
--
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] 315157973 commented on pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1618269104
> @315157973 Could you share the lock profile flame graph before and after this change? +1 for yubiao's concern about the lock
My computer cannot upload pictures due to security rules, and this problem is easy to reproduce.
Use the Producer to send a large number of small packet messages, make sure the CPU of broker is above 60%, and each broker has about 30000 partitions
--
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] 315157973 commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250507578
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
Why not in two steps?
Thread pool isolation is necessary, trim should not run in the main thread pool
Next, let's optimize the time-consuming of trim
--
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] 315157973 commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250785201
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> if yes[Q-1], this patch doesn't have much effect on this issue, right?[Q-2]
Assuming that one trim task increase 0.01ms, if there are tens of thousands of partitions, it will increase a lot
--
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] lifepuzzlefun commented on pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "lifepuzzlefun (via GitHub)" <gi...@apache.org>.
lifepuzzlefun commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1611613347
hi, i notice the scheduleExecutor in `ManagedLedgerImpl` is `OrderedScheduler` do we need to pin the logic on one thread ? like `executor` see https://github.com/apache/pulsar/blob/2b01f83ea4f3ab7b22f4ea2b0290cc183e79bbb8/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L347-L348
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> This lock is segmented and does not block each producer for a long time.
Yes, the task `trim ledgers` has two sub-tasks: Memory changes and Persist changes, and Memory changes would take the lock, Persist changes is an async task that will not take the lock.
But the sub-task Persist changes will not use the thread `executor`, right?<sup>[Q-1]</sup>
if yes<sup>[Q-1]</sup>, this patch doesn't have much effect on this issue, right?<sup>[Q-2]</sup>
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> This lock is segmented and does not block each producer for a long time.
Yes, the task `trim ledgers` has two sub-tasks: Memory changes and Persist changes, and Memory changes would take the lock, Persist changes is an async task that will not take the lock.
---
But the sub-task Persist changes will not use the thread `executor`, right?<sup>[Q-1]</sup>
---
if yes<sup>[Q-1]</sup>, this patch doesn't have much effect on this issue, right?<sup>[Q-2]</sup>
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> This lock is segmented and does not block each producer for a long time.
Make sense, the task `trim ledgers` has two sub-tasks: Memory changes and Persist changes, and Memory changes would take the lock, Persist changes is an async task that will not take the lock.
---
But the sub-task Persist changes will not use the thread `executor`, right?<sup>[Q-1]</sup>
---
if yes<sup>[Q-1]</sup>, this patch doesn't have much effect on this issue, right?<sup>[Q-2]</sup>
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> This lock is segmented and does not block each producer for a long time.
Make sense. The task `trim ledgers` has two sub-tasks: Memory changes and Persist changes, and Memory changes would take the lock, Persist changes is an async task that will not take the lock
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1251691609
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
This change can improve the performance if there are a lot of topics in the broker and there are few threads in the BK client thread pool.
Make sense to me.
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1251003645
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
@315157973
> Assuming that one trim task increase 0.01ms, if there are tens of thousands of partitions, it will increase a lot.
Especially when there are many small packets sent to the cluster， this situation will get worse
I agree with you. But I'm wondering if the current changes will solve the issue.
In the picture above, the logic of `trim ledgers` can be split into two sections: Verification(not in the lock block) and Execution(in the lock block), and there have some async tasks in Section Execution which will run in other threads. The three tasks execute as the flow below.
| Without lock | In lock | In other threads |
| --- | --- | --- |
| Verification | Memory changes | Persist changes |
Since there is the lock `synchronized(ml)`, we can only reduce the executor thread cost of the task `Verification`, which is very simple. It only tries to acquire the lock and is done if it fails.
**(Highlight)** However, this change also makes the scramble of the lock `synchronized(ml)` more frequent, which can worsen performance.
For example, the `topic-a` and `topic-b` were assigned the same thread: `executor-1.` And the task `trim ledgers` will be executed in the thread `scheduled executor`, these tasks will be executed as flow below
| time |`executor-1` | `scheduled executor` |
| --- | --- | --- |
| 1 | publish of `topic-a` |
| 2 | publish of `topic-b` |
| 3 | publish of `topic-b` | start `trim ledgers` of `topic-a` |
| 4 | publish of `topic-b` | get the ML lock of `topic-a` |
| 5 | publish of `topic-b` |
| 6 | publish of `topic-a` |
| 7 | waiting the lock, all publish of `topic-a` and `topic-b` will be blocked util the task `trim ledgers` of `topic-a` is complete |
So in step-4, if the task `trim ledgers` of `topic-a` is executed, the performance will be better; if the task `trim ledgers` of `topic-a` is executed, the performance will be worse. If more and more topics are assigned to the same thread, the performance will be better(maybe we can add a config to disable this feature, it is helpful for the users who have few topics).
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250532829
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
@315157973
Now the tasks `publish` and `trim ledgers` all run in the thread `executor`( it is a single thread pool, and the tasks of same topic will be executed in the same thread<sup>[L-1]</sup> ), right?<sup>[Q-1]</sup>
---
if yes<sup>[Q-1]</sup>, the task `trim ledgers` blocks the task `publish`, which this PR tries to fix, right?<sup>[Q-2]</sup>
---
if yes<sup>[Q-2]</sup>, but both tasks `trim ledgers`<sup>[L-2]</sup> and `internalAsyncAddEntry`<sup>[L-3]</sup> will try to acquire the same lock `synchronized(ml)`, so the task `trim ledgers` is still blocking the task `publish` even if this patch is merged, right?[Q-3]</sup>
---
**[L-1]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L348
**[L-2]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2608
**[L-3]**: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L798
--
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] poorbarcode commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
@315157973
> This lock is segmented and does not block each producer for a long time.
Make sense, the task `trim ledgers` has two sub-tasks: Memory changes and Persist changes, and Memory changes would take the lock, Persist changes is an async task that will not take the lock.
---
But the sub-task Persist changes will not use the thread `executor`, right?<sup>[Q-1]</sup>
---
if yes<sup>[Q-1]</sup>, this patch doesn't have much effect on this issue, right?<sup>[Q-2]</sup>
--
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] 315157973 commented on a diff in pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250785201
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.execute(() -> internalTrimConsumedLedgers(promise));
+ scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
Review Comment:
> if yes[Q-1], this patch doesn't have much effect on this issue, right?[Q-2]
Assuming that one trim task increase 0.01ms, if there are tens of thousands of partitions, it will increase a lot.
Especially when there are many small packets sent to the cluster， this situation will get worse
--
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] github-actions[bot] commented on pull request #20649: [improve][broker] Avoid producer latency rise during internal ledger trimming operations
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#issuecomment-1700224905
The pr had no activity for 30 days, mark with Stale label.
--
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