Jason918 commented on code in PR #15914:
URL: https://github.com/apache/pulsar/pull/15914#discussion_r983270598
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java:
##########
@@ -873,23 +891,101 @@ public void getInfoFailed(ManagedLedgerException
exception, Object ctx) {
}, ctx);
}
+ private CompletableFuture<Void> cleanupOffloaded(long ledgerId, UUID uuid,
ManagedLedgerConfig mlConfig,
+ Map<String, String>
offloadDriverMetadata, String cleanupReason, String name) {
+ log.info("[{}] Cleanup offload for ledgerId {} uuid {} because of the
reason {}.",
+ name, ledgerId, uuid.toString(), cleanupReason);
+ Map<String, String> metadataMap = new HashMap();
+ metadataMap.putAll(offloadDriverMetadata);
+ metadataMap.put("ManagedLedgerName", name);
+
+ return
Retries.run(Backoff.exponentialJittered(TimeUnit.SECONDS.toMillis(1),
+ TimeUnit.SECONDS.toHours(1)).limit(10),
+ Retries.NonFatalPredicate,
+ () -> mlConfig.getLedgerOffloader().deleteOffloaded(ledgerId,
uuid, metadataMap),
+ scheduledExecutor, name).whenComplete((ignored, exception) -> {
+ if (exception != null) {
+ log.warn("[{}] Error cleaning up offload for {}, (cleanup
reason: {})",
+ name, ledgerId, cleanupReason, exception);
+ }
+ });
+ }
+
private void deleteManagedLedgerData(BookKeeper bkc, String
managedLedgerName, ManagedLedgerInfo info,
- DeleteLedgerCallback callback, Object ctx) {
+
CompletableFuture<ManagedLedgerConfig> mlConfigFuture,
+ DeleteLedgerCallback callback, Object
ctx) {
+ final CompletableFuture<Map<Long,
MLDataFormats.ManagedLedgerInfo.LedgerInfo>>
+ ledgerInfosFuture = new CompletableFuture<>();
+ store.getManagedLedgerInfo(managedLedgerName, false, null,
Review Comment:
> Unfortunately ManagedLedgerInfo is part of of the public REST API
Sorry, I missed this.
> I prefer to keep the patch in this form. And if we want to change
ManagedLedgerInfo we can do it in a follow up work.
>
> As this patch is fixing some kind of "bad problem" (because we are not
deleting data that should have been deleted, that has some legal impact in some
countries), this patch should be cherry-picked to active branches. I won't add
API changes in a patch that will be ported
Make sense to me, this patch LGTM
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
