[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion #15914

dlg99 · 2022-06-02T20:43:06Z

Fixes #9962

Motivation

Offloaded ledgers can be orphaned on topic deletion.

Modifications

Topic to be truncated before deletion to delete offloaded ledgers properly and fail if truncation fails.

Verifying this change

Make sure that the change passes the CI checks.

This change added integration tests

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Nothing changed in the options but admin CLI will implicitly run truncate before topic delete.

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes / no)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

eolivelli

Lgtm.
Great

@hangc0276

hangc0276 · 2022-06-06T01:28:05Z

Even though we should delete the offload data on topic deletion, it has changed the default behavior. We'd better have a proposal to discuss it.

Another one is that it can't prevent orphan ledgers because they offloaded data deletion is async.

hangc0276 · 2022-06-06T01:28:47Z

We need more eyes. @merlimat @codelipenghui @315157973 @Jason918 @zymap @horizonzy

dlg99 · 2022-06-06T17:18:25Z

@hangc0276

Even though we should delete the offload data on topic deletion, it has changed the default behavior. We'd better have a proposal to discuss it.

I disagree that undocumented silently orphaned data (without tools/automated processes to detect it and clean up) is a "default behavior" one ever expected or wanted. It looks like a bug that just happened.

Another one is that it can't prevent orphan ledgers because they offloaded data deletion is async.

In case of truncate internalTrimLedgers runs with isTruncate == true actual future that completes when trimming is done, and the topic deletion will follow that.
In case of deletion we have isTruncate == false and internalTrimLedgers runs with Futures.NULL_PROMISE.

In case I missed some place where internalTrimLedgers completes the promise before the data deletion is done i'll fix that as long as we agree on overall approach.

horizonzy · 2022-06-10T08:44:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

+                                    // Truncate to ensure the offloaded data is not orphaned.
+                                    // Also ensures the BK ledgers are deleted and not just scheduled for deletion
+                                    CompletableFuture<Void> truncateFuture = ledger.asyncTruncate();
+                                    truncateFuture.whenComplete((ignore, exc) -> {


How about that move this logic to ledger.asyncDelete(). It can cover more situation.

@horizonzy moved it into ManagedLedgerImpl

hangc0276

Overall LGTM.

My only concern is if the offload data have been used by other systems, for example, a hive table, the topic deletion will delete the table, and other systems won't get the data out from the table.

There is another risk is that after the ledger deletion succeeds, but some ledgers deletion from BookKeeper failed, it won't delete the topic metadata, which will lead to some ledgers has been deleted from storage, but their metadata still can be found from the topic metadata, and the consumer will fetch data failed. We are writing a proposal to solve this issue.

horizonzy · 2022-06-26T02:24:59Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

@@ -2687,6 +2687,22 @@ public void deleteLedgerFailed(ManagedLedgerException e, Object ctx) {

    @Override
    public void asyncDelete(final DeleteLedgerCallback callback, final Object ctx) {
+        // Truncate to ensure the offloaded data is not orphaned.
+        // Also ensures the BK ledgers are deleted and not just scheduled for deletion
+        CompletableFuture<Void> truncateFuture = asyncTruncate();


If we use asyncTruncate to trigger delete storage data.
Maybe we should mofidy the code:

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

Lines 2430 to 2434 in ce24db1

if (!factory.isMetadataServiceAvailable()) {

// Defer trimming of ledger if we cannot connect to metadata service

promise.complete(null);

return;

}

When the meta data service is not available, should complete with exception.

github-actions · 2022-07-26T02:54:43Z

The pr had no activity for 30 days, mark with Stale label.

dlg99 · 2022-09-16T18:36:44Z

@eolivelli @hangc0276 please take another look.
I addressed the case when the topic is unloaded.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

Jason918 · 2022-09-17T04:00:56Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java

+                                         DeleteLedgerCallback callback, Object ctx) {
+        final CompletableFuture<Map<Long, MLDataFormats.ManagedLedgerInfo.LedgerInfo>>
+                ledgerInfosFuture = new CompletableFuture<>();
+        store.getManagedLedgerInfo(managedLedgerName, false, null,


Why do we read ManagedLedgerInfo from store instead of just use info in the parameter list?

info is org.apache.bookkeeper.mledger.ManagedLedgerInfo, store returns MLDataFormats.ManagedLedgerInfo.LedgerInfo which has some additional info and used in OffloadUtils

@dlg99 This still confuse me. All data in ManagedLedgerInfo comes directly from MLDataFormats.ManagedLedgerInfo.LedgerInfo. I think it's better to just sync all the info to ManagedLedgerInfo.

Unfortunately ManagedLedgerInfo is part of of the public REST API

for instance here we return it (JSON encoded) to the client

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

Line 1262 in 957a16b

PartitionedManagedLedgerInfo partitionedManagedLedgerInfo = new PartitionedManagedLedgerInfo();

We should not add everything to it.

We have this bad problem in Pulsar that we aren't always aware of what is leaking to the public APIs.

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

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

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerFactory.java

dlg99 · 2022-09-22T02:32:43Z

@Jason918 I addressed your comments

eolivelli

LGTM

…pic Deletion (apache#15914)" This reverts commit 9026d19.

eolivelli · 2022-09-29T12:06:22Z

tests are now failing due to #17736

because "trim" cannot happen if a ManagedLedger is "fenced", and we set "fenced" in "delete".
so there is some code path in which we "delete" and then try to "trim".
we should do the opposite...trim and then delete.

…pic Deletion (#15914)" (#17889) This reverts commit 9026d19.

…tion (apache#15914) * Truncate topic before deletion to avoid orphaned offloaded ledgers * CR feedback (cherry picked from commit 9026d19)

@eolivelli

…tion (apache#17915) (#152) (cherry picked from commit 0854032) Fixes apache#9962 ### Motivation Offloaded ledgers can be orphaned on topic deletion. This is a redo of apache#15914 which conflicted with concurrently merged apache#17736 thus resulting in apache#17889 . apache#17736 made a decision to not allow managed ledger trimming for the fenced mledgers because in many case fencing indicates a problems that should stop all operations on mledger. At the same time fencing is used before deletion starts, so trimming added to the deletion process cannot proceed. After discussion with @eolivelli I introduced new state, FencedForDeletion, which acts as Fenced state except for the trimming/deletion purposes. ### Modifications Topic to be truncated before deletion to delete offloaded ledgers properly and fail if truncation fails. ### Verifying this change local fork tests: dlg99#1 - [ ] Make sure that the change passes the CI checks. This change added integration tests ### Does this pull request potentially affect one of the following parts: *If `yes` was chosen, please highlight the changes* Nothing changed in the options but admin CLI will implicitly run truncate before topic delete. - Dependencies (does it add or upgrade a dependency): (yes / no) - The public API: (yes / no) - The schema: (yes / no / don't know) - The default values of configurations: (yes / no) - The wire protocol: (yes / no) - The rest endpoints: (yes / no) - The admin cli options: (yes / no) - Anything that affects deployment: (yes / no / don't know) ### Documentation Check the box below or label this PR directly. Need to update docs? - [ ] `doc-required` (Your PR needs to update docs and you will update later) - [x] `doc-not-needed` (Please explain why) - [ ] `doc` (Your PR contains doc changes) - [ ] `doc-complete` (Docs have been already added)

congbobo184 · 2022-11-15T02:06:22Z

@dlg99 could you please cherry-pick this PR to branch-2.9? thanks.

congbobo184 · 2022-11-17T11:43:34Z

@dlg99 hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.

…tion (#15914) * Truncate topic before deletion to avoid orphaned offloaded ledgers * CR feedback (cherry picked from commit 9026d19)

tisonkun · 2023-02-28T05:15:11Z

Remove the release/* tags since it changes the default behavior and should not go into early releases.

github-actions bot assigned dlg99 Jun 2, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 2, 2022

eolivelli approved these changes Jun 3, 2022

View reviewed changes

eolivelli requested review from codelipenghui, hangc0276, merlimat, Jason918, 315157973 and zymap June 6, 2022 13:54

horizonzy reviewed Jun 10, 2022

View reviewed changes

dlg99 marked this pull request as draft June 22, 2022 04:07

eolivelli approved these changes Jun 22, 2022

View reviewed changes

hangc0276 approved these changes Jun 22, 2022

View reviewed changes

horizonzy reviewed Jun 26, 2022

View reviewed changes

github-actions bot added the Stale label Jul 26, 2022

dlg99 force-pushed the topic-deletion branch 2 times, most recently from 75c7192 to a912495 Compare September 14, 2022 01:06

dlg99 marked this pull request as ready for review September 15, 2022 21:13

github-actions bot removed the Stale label Sep 17, 2022

Jason918 reviewed Sep 17, 2022

View reviewed changes

dlg99 added 2 commits September 21, 2022 15:06

Truncate topic before deletion to avoid orphaned offloaded ledgers

9d9c63b

CR feedback

7c63673

dlg99 force-pushed the topic-deletion branch from a912495 to 7c63673 Compare September 21, 2022 22:47

eolivelli approved these changes Sep 29, 2022

View reviewed changes

Jason918 added the release/2.10.3 label Sep 29, 2022

eolivelli merged commit 9026d19 into apache:master Sep 29, 2022

eolivelli added a commit to eolivelli/pulsar that referenced this pull request Sep 29, 2022

Revert "[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On To…

84f59d2

…pic Deletion (apache#15914)" This reverts commit 9026d19.

eolivelli mentioned this pull request Sep 29, 2022

Revert "[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion (#15914)" #17889

Merged

merlimat pushed a commit that referenced this pull request Sep 29, 2022

Revert "[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On To…

f0b6348

…pic Deletion (#15914)" (#17889) This reverts commit 9026d19.

dlg99 mentioned this pull request Oct 4, 2022

[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion #17915

Merged

5 tasks

dlg99 mentioned this pull request Oct 25, 2022

[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion (#17915) datastax/pulsar#152

Merged

5 tasks

congbobo184 added release/2.9.5 and removed release/2.9.4 labels Nov 17, 2022

liangyepianzhou added release/2.10.4 and removed release/2.10.3 labels Dec 13, 2022

liangyepianzhou mentioned this pull request Feb 26, 2023

[branch-2.10][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion (#15914) #19640

Closed

15 tasks

liangyepianzhou added release/2.10.5 and removed release/2.10.4 labels Feb 26, 2023

coderzc added release/2.9.6 and removed release/2.9.5 labels Feb 27, 2023

codelipenghui added the release/important-notice The changes which are important should be mentioned in the release note label Feb 28, 2023

tisonkun removed release/2.8.5 release/2.11.1 release/2.10.5 release/2.9.6 labels Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion #15914

[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion #15914

dlg99 commented Jun 2, 2022

eolivelli left a comment

hangc0276 commented Jun 6, 2022

hangc0276 commented Jun 6, 2022 •

edited

Loading

dlg99 commented Jun 6, 2022

horizonzy Jun 10, 2022 •

edited

Loading

dlg99 Jun 22, 2022

hangc0276 left a comment

horizonzy Jun 26, 2022

github-actions bot commented Jul 26, 2022

dlg99 commented Sep 16, 2022

Jason918 Sep 17, 2022

dlg99 Sep 21, 2022

Jason918 Sep 24, 2022 •

edited

Loading

eolivelli Sep 29, 2022

Jason918 Sep 29, 2022 •

edited

Loading

dlg99 commented Sep 22, 2022

eolivelli left a comment

eolivelli commented Sep 29, 2022

congbobo184 commented Nov 15, 2022 •

edited

Loading

congbobo184 commented Nov 17, 2022

tisonkun commented Feb 28, 2023

	if (!factory.isMetadataServiceAvailable()) {
	// Defer trimming of ledger if we cannot connect to metadata service
	promise.complete(null);
	return;
	}

[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion #15914

[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Deletion #15914

Conversation

dlg99 commented Jun 2, 2022

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

eolivelli left a comment

Choose a reason for hiding this comment

hangc0276 commented Jun 6, 2022

hangc0276 commented Jun 6, 2022 • edited Loading

dlg99 commented Jun 6, 2022

horizonzy Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

dlg99 Jun 22, 2022

Choose a reason for hiding this comment

hangc0276 left a comment

Choose a reason for hiding this comment

horizonzy Jun 26, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 26, 2022

dlg99 commented Sep 16, 2022

Jason918 Sep 17, 2022

Choose a reason for hiding this comment

dlg99 Sep 21, 2022

Choose a reason for hiding this comment

Jason918 Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

eolivelli Sep 29, 2022

Choose a reason for hiding this comment

Jason918 Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

dlg99 commented Sep 22, 2022

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Sep 29, 2022

congbobo184 commented Nov 15, 2022 • edited Loading

congbobo184 commented Nov 17, 2022

tisonkun commented Feb 28, 2023

hangc0276 commented Jun 6, 2022 •

edited

Loading

horizonzy Jun 10, 2022 •

edited

Loading

Jason918 Sep 24, 2022 •

edited

Loading

Jason918 Sep 29, 2022 •

edited

Loading

congbobo184 commented Nov 15, 2022 •

edited

Loading