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 2022/03/03 11:28:51 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14545: Cancel offload tasks when managed ledger closed.
Technoboy- opened a new pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545
### Motivation
When the user config the offloader, as the ledger close, it will trigger the ledger to offload. If there are many ledgers that need to offload, but the topic has been unloaded, the offloader will continue to offload. Because the offloader uses the shared executor pool in ManagedLedgerFactoryImpl and when the managed ledger closes, it doesn't cancel the tasks.
```
15:29:59.180 [pulsar-web-41-3] INFO org.apache.pulsar.broker.admin.impl.PersistentTopicsBase - [null] Unloading topic persistent://public/default/UpdateNodeCharts
15:29:59.201 [pulsar-web-41-3] INFO org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl - [public/default/persistent/UpdateNodeCharts] Closing managed ledger
15:29:59.216 [main-EventThread] INFO org.apache.bookkeeper.mledger.impl.MetaStoreImpl - [public/default/persistent/UpdateNodeCharts] [cloud-nodes-service] Updating cursor info ledgerId=-1 mark-delete=789182:82011
15:29:59.219 [bookkeeper-ml-scheduler-OrderedScheduler-4-0] INFO org.apache.bookkeeper.mledger.impl.ManagedCursorImpl - [public/default/persistent/UpdateNodeCharts][cloud-nodes-service] Closed cursor at md-position=789182:82011
15:29:59.221 [bookkeeper-ml-scheduler-OrderedScheduler-4-0] INFO org.apache.pulsar.broker.service.persistent.PersistentTopic - [persistent://public/default/UpdateNodeCharts] Topic closed
15:29:59.221 [bookkeeper-ml-scheduler-OrderedScheduler-4-0] INFO org.apache.pulsar.broker.admin.impl.PersistentTopicsBase - [null] Successfully unloaded topic persistent://public/default/UpdateNodeCharts
15:31:05.432 [offloader-OrderedScheduler-1-0] INFO org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl - [public/default/persistent/UpdateNodeCharts] Preparing metadata to offload ledger 422142 with uuid 030267e2-a2f9-40a3-848b-482f9b007c00
15:31:05.432 [offloader-OrderedScheduler-1-0] INFO org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl - [public/default/persistent/UpdateNodeCharts] Found previous offload attempt for ledger 422142, uuid 030267e2-a2f9-40a3-848b-482f9b007c00, cleaning up
15:31:05.432 [offloader-OrderedScheduler-1-0] INFO org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl - [public/default/persistent/UpdateNodeCharts] Cleanup offload for ledgerId 422142 uuid 3725b3c1-1dbc-481f-a1dd-8aaffb75e603 because of the reason Previous failed offload.
```
### Modifications
- Cancel offloader tasks when managed ledger close.
### Documentation
- [x] `no-need-doc`
--
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] Technoboy- commented on pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#issuecomment-1060281524
Hi @Jason918 @gaoran10 Could you help review this ?
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r823587850
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
Is the change necessary? After this change, `offloadLoop` won't be completed exceptionally if the `toOffload` is empty when the managed ledger is closed.
--
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] Technoboy- commented on a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r824322948
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
Yes, the behavior is the same, but only make it more readable.
--
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] Jason918 commented on a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r820570563
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2317,7 +2317,7 @@ private void maybeOffloadInBackground(CompletableFuture<PositionImpl> promise) {
private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
if (!offloadMutex.tryLock()) {
- scheduledExecutor.schedule(safeRun(() -> maybeOffloadInBackground(finalPromise)),
+ scheduledExecutor.schedule(safeRun(() -> maybeOffload(finalPromise)),
Review comment:
Why do we change this？
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r823807052
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
I see. But the code here is very weird. It looks like the result of the future has no effect because `maybeOffload` accepts `NULL_OFFLOAD_PROMISE`, which is a completed future.
```java
unlockingPromise.whenComplete((res, ex) -> {
offloadMutex.unlock();
// The following code is meaningless because `finalPromise` is already completed
if (ex != null) {
finalPromise.completeExceptionally(ex);
} else {
finalPromise.complete(res);
}
});
```
And I've checked the calls of `maybeOffload`, it is only called in `maybeOffloadInBackground`, which could be called by:
1. `ledgerClosed`: the future is `NULL_OFFLOAD_PROMISE`.
2. `maybeOffload` when locking the `offloadMutex` successfully: since `maybeOffload` could only be called in `maybeOffloadInBackground`, the future must be `NULL_OFFLOAD_PROMISE` as well.
The code design here is really confused.
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r824324610
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
But it has the comments:
> // offloadLoop will complete immediately with an empty list to offload
--
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] Technoboy- commented on a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r820625116
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2317,7 +2317,7 @@ private void maybeOffloadInBackground(CompletableFuture<PositionImpl> promise) {
private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
if (!offloadMutex.tryLock()) {
- scheduledExecutor.schedule(safeRun(() -> maybeOffloadInBackground(finalPromise)),
+ scheduledExecutor.schedule(safeRun(() -> maybeOffload(finalPromise)),
Review comment:
Ok, this will make race condition. Rollback.
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r823609513
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
If the managed ledger has been closed, and the toOffload is empty, we just complete the future has no bad effect.
--
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] Technoboy- commented on a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r824322948
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
Yes, the behavior is the same, but only make it not confused.
The original seems to do `offloadLoop` in any 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] congbobo184 merged pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
congbobo184 merged pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r823808996
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
If whether `unlockingPromise` is completed exceptionally makes no difference. Changing the current behavior seems useless.
--
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 #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#issuecomment-1059180091
/pulsarbot run-failure-checks
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r823808996
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
If whether `unlockingPromise` is completed exceptionally makes no difference, changing the current behavior seems useless.
IMO, a PR should usually avoid the unrelated changes. Yes we can treat the empty `ledgersToOffload` case as no error, even if the state is `Closed`. However, what does it affect after making this change?
--
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 a change in pull request #14545: Cancel offload tasks when managed ledger closed.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14545:
URL: https://github.com/apache/pulsar/pull/14545#discussion_r823808996
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2363,13 +2363,13 @@ private void maybeOffload(CompletableFuture<PositionImpl> finalPromise) {
+ ", total size = {}, already offloaded = {}, to offload = {}",
name, toOffload.stream().map(LedgerInfo::getLedgerId).collect(Collectors.toList()),
sizeSummed, alreadyOffloadedSize, toOffloadSize);
+ offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
} else {
// offloadLoop will complete immediately with an empty list to offload
log.debug("[{}] Nothing to offload, total size = {}, already offloaded = {}, threshold = {}",
name, sizeSummed, alreadyOffloadedSize, threshold);
+ unlockingPromise.complete(PositionImpl.LATEST);
}
-
- offloadLoop(unlockingPromise, toOffload, PositionImpl.LATEST, Optional.empty());
Review comment:
If whether `unlockingPromise` is completed exceptionally makes no difference, changing the current behavior seems useless.
--
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