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/07/20 07:59:37 UTC
[GitHub] [pulsar] lhotari opened a new pull request #11387: [ManagedLedger] Don't expose OrderedExecutor from ManagedLedgerImpl.getExecutor
lhotari opened a new pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387
### Motivation
OpReadEntry is not multi-thread safe. OpReadEntry.entries is an ArrayList without any synchronization.
However it is accessed from multiple threads.
Here's an example of such code:
OpReadEntry.entries mutated:
https://github.com/apache/pulsar/blob/d383b22a63af45d810111af2e502e5c26774f82e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L81
Mutation triggered from multiple threads:
https://github.com/apache/pulsar/blob/d383b22a63af45d810111af2e502e5c26774f82e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L137-L148
The operations seem to happen sequentially when OpReadEntry.entries is mutated or accessed, but this is not enough for ensuring thread safety in Java.
The goal of this change is to improve Managed Ledger operations by running operations in the pinned executor thread where the thread is picked by the hash of the managed ledger name. This is how most of the code is already written, but there are some exceptions in the current code. **The goal of this PR change is to ensure that the pinned executor is used in all cases where work is scheduled to run using the ledger's executor.**
### Modifications
- Don't expose OrderedExecutor from ManagedLedgerImpl.getExecutor
- instead, return executor that is pinned to a single thread with `.chooseThread(getName())`
- most usages of ManagedLedgerImpl.getExecutor were already calling `.chooseThread(ml.getName())`, however
some locations were omitting it. It's better to always pin the ManagedLedgerImpl.getExecutor
to a single thread.
- this improves thread safety of Managed Ledger code base since more operations will happen in a single thread
- some classes such as OpReadEntry are not multi-thread safe. OpReadEntry.entries is a ArrayList without any synchronization.
--
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] devinbost commented on pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
devinbost commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883722044
Related to #6054
--
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] lhotari commented on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883284819
--
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 #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r673164277
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
Review comment:
This is a behaviour change
how can we verify that we are not breaking something or reducing overall performances ?
--
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] BewareMyPower commented on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883265034
--
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] lhotari commented on pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883316287
> @lhotari I know. What I want to ask is when `ManagedCursor#asyncReadEntries` is called, the `OpReadEntry#getNumberOfEntriesToRead` will be called eventually even after your change. Is it called in another thread? If yes, there could still be a thread safety problem.
The currency design doesn't currently ensure single thread access. It's possible that there is multi-thread access at the moment. For example, the scheduler uses a different executor and thread. However, I believe that this PR improves the existing solution and can help reduce issues caused by thread safety issues.
I think it's possible to incrementally refactor the concurrency solution towards a solution where a single thread handles all accesses for a specific key (managed ledger name in this case). We can have a broader discussion about this on the mailing list or in some upcoming community meeting.
--
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] merlimat commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r696987788
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ // Schedule next read
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
> Wouldn't the usage of the pinned executor achieve the same result? It prevents the stack from going deeper and >deeper.
> Why would it have to jump to a random thread to break the chain?
@lhotari Uhm, I think that some executors are short-circuiting the queue if they detect that you're trying to add a task from the same executor thread. That is the case for Netty IO thread, though I just check that it shouldn't happen on the `ThreadPoolExecutor` on which the OrderedExecutor is based upon.
--
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] merlimat commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r696888143
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -252,7 +253,8 @@
protected volatile State state = null;
private final OrderedScheduler scheduledExecutor;
- private final OrderedExecutor executor;
+ private final ScheduledExecutorService pinnedScheduledExecutor;
+ private final Executor pinnedExecutor;
Review comment:
> Perhaps a more optimal solution would be to have the capability for scheduling tasks on the pinned scheduler
@lhotari The scheduled executor is less efficient compared to the normal executor because it has to maintain the delayed tasks. For that it's preferable not to use it directly in the critical data path, but only when we want to defer actions or for background tasks.
--
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] lhotari commented on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883284819
> Could `ManagedCursor#asyncReadEntries` be called in a different thread?
This is one example in the current code base where a different thread might be used:
https://github.com/apache/pulsar/blob/5ad405988fabb4b28dbdbd5aa5c9a10802f39af1/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L137-L148
--
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] lhotari closed pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387
--
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] lhotari commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r694619505
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ // Schedule next read
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
Wouldn't the usage of the pinned executor achieve the same result? It prevents the stack from going deeper and deeper.
Why would it have to jump to a random thread to break the chain?
The only reason that comes into my mind is the case where there's a completable future that gets triggered as part of teh call flow and it is being waited to complete in the same thread as where the result should be executed in. That would never complete and would dead lock. Would that be the case here to use a different executor?
--
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] BewareMyPower commented on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883265034
Great find!
But I have a question about it. `entries` is exposed by followed public method:
https://github.com/apache/pulsar/blob/5ad405988fabb4b28dbdbd5aa5c9a10802f39af1/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L163-L165
And the method's call stack could be:
```
ManagedCursor#asyncReadEntries
ManagedCursorImpl#asyncReadEntries
ManagedLedgerImpl#asyncReadEntries
ManagedLedgerImpl#internalReadFromLedger
```
Could `ManagedCursor#asyncReadEntries` be called in a different thread?
--
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] lhotari commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r694651964
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -93,7 +93,7 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
if (!entries.isEmpty()) {
// There were already some entries that were read before, we can return them
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
Yes that's true.
The reason to use the pinned executor is to adhere to Java Memory Model rules of correct synchronization. There's a generic problem in OpReadEntry since it's sharing an array that is mutated by multiple threads. [JLS 17.4 explains that "Incorrectly Synchronized Programs May Exhibit Surprising Behavior"](https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4).
I would assume that "entries" would have to be copied to a new list before sharing if we want to use multiple threads. Is that right?
--
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] lhotari edited a comment on pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883316287
> @lhotari I know. What I want to ask is when `ManagedCursor#asyncReadEntries` is called, the `OpReadEntry#getNumberOfEntriesToRead` will be called eventually even after your change. Is it called in another thread? If yes, there could still be a thread safety problem.
The concurrency design doesn't currently ensure single thread access. It's possible that there is multi-thread access at the moment. For example, the scheduler uses a different executor and thread. However, I believe that this PR improves the existing solution and can help reduce issues caused by thread safety issues.
I think it's possible to incrementally refactor the concurrency solution towards a solution where a single thread handles all accesses for a specific key (managed ledger name in this case). We can have a broader discussion about this on the mailing list or in some upcoming community meeting.
--
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] lhotari commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r673291804
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
Review comment:
> how can we verify that we are not breaking something or reducing overall performances ?
testing, testing, testing. we need more Fallout tests. :)
--
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] lhotari edited a comment on pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883316287
> @lhotari I know. What I want to ask is when `ManagedCursor#asyncReadEntries` is called, the `OpReadEntry#getNumberOfEntriesToRead` will be called eventually even after your change. Is it called in another thread? If yes, there could still be a thread safety problem.
The concurrency design doesn't currently ensure single thread access. It's possible that there is multi-thread access at the moment. For example, the scheduler uses a different executor and therefore also a different thread. However, I believe that this PR improves the existing solution and can help reduce issues caused by thread safety issues.
I think it's possible to incrementally refactor the concurrency solution towards a solution where a single thread handles all accesses for a specific key (managed ledger name in this case). We can have a broader discussion about this on the mailing list or in some upcoming community meeting.
--
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] merlimat commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r693112234
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -252,7 +253,8 @@
protected volatile State state = null;
private final OrderedScheduler scheduledExecutor;
- private final OrderedExecutor executor;
+ private final ScheduledExecutorService pinnedScheduledExecutor;
+ private final Executor pinnedExecutor;
Review comment:
If we are using 2 threads, one with the regular executor (which is more efficient) and the other for the `pinnedScheduledExecutor`, wouldn't that mean that we still have more than 1 thread accessing some of the objects?
Would it make sense to use the generic `scheduledExecutor` (just for deferring purposes) and then jump back into the same `pinnedExecutor`?
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2159,7 +2162,7 @@ void notifyCursors() {
break;
}
- executor.execute(safeRun(waitingCursor::notifyEntriesAvailable));
+ pinnedExecutor.execute(safeRun(waitingCursor::notifyEntriesAvailable));
Review comment:
Is this required to be on the same executor?
We're notify multiple cursors that entries are available, this should be able to progress in parallel.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -93,7 +93,7 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
if (!entries.isEmpty()) {
// There were already some entries that were read before, we can return them
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
I think we should be careful in not serializing every cursor into the managed ledger pinned thread, as it could become a bottleneck where there are many cursors on a topic.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ // Schedule next read
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
Other than the consideration that different cursors shouldn't be pinned on a single thread, the reason for jumping to a different thread here is to avoid a stack overflow.
When the read is being served from the ML cache, it's coming back from same thread. There are some conditions in which we ask for next read.
eg. If you ask to read 100 entries and we only got 20 entries from current ledger, we'll schedule a read for the remaining 80 on next ledger. In some cases there could be abnormal distributions, like 1 entry per ledger and it would be chaining all the reads and callback within the same stack.
Therefore, the "jump to a random thread" was introduced to break that chain.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2217,15 +2220,16 @@ private void trimConsumedLedgersInBackground() {
@Override
public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
- executor.executeOrdered(name, safeRun(() -> internalTrimConsumedLedgers(promise)));
+ pinnedExecutor.execute(safeRun(() -> internalTrimConsumedLedgers(promise)));
}
public void trimConsumedLedgersInBackground(boolean isTruncate, CompletableFuture<?> promise) {
- executor.executeOrdered(name, safeRun(() -> internalTrimLedgers(isTruncate, promise)));
+ pinnedExecutor.execute(safeRun(() -> internalTrimLedgers(isTruncate, promise)));
}
private void scheduleDeferredTrimming(boolean isTruncate, CompletableFuture<?> promise) {
- scheduledExecutor.schedule(safeRun(() -> trimConsumedLedgersInBackground(isTruncate, promise)), 100, TimeUnit.MILLISECONDS);
+ pinnedScheduledExecutor
Review comment:
Since `trimConsumedLedgersInBackground()` is already jumping on the `pinnedExecutor`, we shouldn't need to use a specific thread for the scheduled executor.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2170,7 +2173,7 @@ void notifyWaitingEntryCallBacks() {
break;
}
- executor.execute(safeRun(cb::entriesAvailable));
+ pinnedExecutor.execute(safeRun(cb::entriesAvailable));
Review comment:
Same for this one, it should be same to spread into multiple threads.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -298,7 +300,8 @@ public ManagedLedgerImpl(ManagedLedgerFactoryImpl factory, BookKeeper bookKeeper
this.ledgerMetadata = LedgerMetadataUtils.buildBaseManagedLedgerMetadata(name);
this.digestType = BookKeeper.DigestType.fromApiDigestType(config.getDigestType());
this.scheduledExecutor = scheduledExecutor;
- this.executor = bookKeeper.getMainWorkerPool();
+ this.pinnedScheduledExecutor = scheduledExecutor.chooseThread(name);
+ this.pinnedExecutor = bookKeeper.getMainWorkerPool().chooseThread(name);
Review comment:
I don't know why we never did this, but this saves a lot of string hashings too :)
--
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 a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r694736671
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2159,7 +2162,7 @@ void notifyCursors() {
break;
}
- executor.execute(safeRun(waitingCursor::notifyEntriesAvailable));
+ pinnedExecutor.execute(safeRun(waitingCursor::notifyEntriesAvailable));
Review comment:
There are 2 places will call the `notifyCursors()` method, one is OpAddEntry.safeRun(), it already run the `pinnedExecutor` so don't need to jump again.
Another one is the ledger closed, looks only need to change here.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -252,7 +253,8 @@
protected volatile State state = null;
private final OrderedScheduler scheduledExecutor;
- private final OrderedExecutor executor;
+ private final ScheduledExecutorService pinnedScheduledExecutor;
+ private final Executor pinnedExecutor;
Review comment:
> Would it make sense to use the generic scheduledExecutor (just for deferring purposes) and then jump back into the same pinnedExecutor?
@merlimat Do you mean scheduledExecutor.schedule(pinnedExecutor.execute() ...) ?
Seems to be a feasible way right now :)
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ // Schedule next read
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
It looks like the stack should be `checkReadCompletion -> entryCache.asyncReadEntry0 -> checkReadCompletion -> entryCache.asyncReadEntry0 -> checkReadCompletion -> entryCache.asyncReadEntry0` and so on, if we have entries in the cache.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -93,7 +93,7 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
if (!entries.isEmpty()) {
// There were already some entries that were read before, we can return them
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
I think the entry reading happens one by one, if we got the read entries failed here, this means we will not get a chance to add more elements to the list right(all the previous read operations are done)?
--
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] lhotari commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r694619505
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
##########
@@ -141,8 +141,8 @@ void checkReadCompletion() {
cursor.ledger.startReadOperationOnLedger(nextReadPosition, OpReadEntry.this);
}
- // Schedule next read in a different thread
- cursor.ledger.getExecutor().execute(safeRun(() -> {
+ // Schedule next read
+ cursor.ledger.getPinnedExecutor().execute(safeRun(() -> {
Review comment:
Wouldn't the usage of the pinned executor achieve the same result? It prevents the stack from going deeper and deeper.
Why would it have to jump to a random thread to break the chain?
The only reason that comes into my mind is the case where there's a completable future that gets triggered as part of the call flow and it is being waited to complete in the same thread as where the result should be executed in. That would never complete and would dead lock. Would that be the case here to use a different executor?
--
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] BewareMyPower commented on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883302914
@lhotari I know. What I want to ask is when `ManagedCursor#asyncReadEntries` is called, the `OpReadEntry#getNumberOfEntriesToRead` will be called eventually even after your change. Is it called in another thread? If yes, there could still be a thread safety problem.
--
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] lhotari edited a comment on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883284819
--
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] lhotari commented on a change in pull request #11387: [ManagedLedger] Pin executor and scheduled executor threads for ManagedLedgerImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#discussion_r694672599
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -252,7 +253,8 @@
protected volatile State state = null;
private final OrderedScheduler scheduledExecutor;
- private final OrderedExecutor executor;
+ private final ScheduledExecutorService pinnedScheduledExecutor;
+ private final Executor pinnedExecutor;
Review comment:
That's true.
Perhaps a more optimal solution would be to have the capability for scheduling tasks on the pinned scheduler. I don't know why this solution isn't available in the underlying Bookkeeper libraries that are used. The benefit of that is that there isn't an additional thread switch when the scheduled task triggers.
--
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] lhotari edited a comment on pull request #11387: [ManagedLedger] Pin executor thread for usages of ManagedLedgerImpl.getExecutor
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #11387:
URL: https://github.com/apache/pulsar/pull/11387#issuecomment-883284819
> Could `ManagedCursor#asyncReadEntries` be called in a different thread?
@BewareMyPower This is one example in the current code base where a different thread might be used:
https://github.com/apache/pulsar/blob/5ad405988fabb4b28dbdbd5aa5c9a10802f39af1/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L137-L148
--
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