Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Sep 29, 2022
Merged

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

merged 1 commit into from
Sep 29, 2022

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Sep 29, 2022 •
edited
Loading

This reverts commit 9026d19.

PR is #15914

@eolivelli eolivelli self-assigned this Sep 29, 2022
@eolivelli eolivelli added the doc-not-needed Your PR changes do not impact docs label Sep 29, 2022
@mattisonchao
Copy link
Member

Hi @eolivelli

Could you tell us why we have to revert it?

@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Sep 29, 2022
@github-actions
Copy link

@eolivelli Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@eolivelli
Copy link
Contributor Author

@cbornet may add more details.
A few tests on master branch are now failing

@lhotari lhotari closed this Sep 29, 2022
@lhotari lhotari reopened this Sep 29, 2022
@cbornet
Copy link
Contributor

cbornet commented Sep 29, 2022 •
edited
Loading

Master fails consistently on several tests:
testGetAllPartitions(org.apache.pulsar.broker.namespace.NamespaceOwnershipListenerTests)
producerBacklogQuotaExceededTest(org.apache.pulsar.websocket.proxy.ProxyPublishConsumeTest)
testDeleteNamespace(org.apache.pulsar.broker.admin.NamespacesTest)

I have the same failure locally on latest master.
Can someone else confirm ?

@eolivelli eolivelli added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Sep 29, 2022
@eolivelli
Copy link
Contributor Author

mvn test -pl pulsar-broker -Dtest=NamespaceOwnershipListenerTests

Message: org.apache.bookkeeper.mledger.ManagedLedgerException$ManagedLedgerFencedException: Can't trim fenced ledger

Stacktrace:

org.apache.pulsar.broker.service.BrokerServiceException$PersistenceException: org.apache.bookkeeper.mledger.ManagedLedgerException$ManagedLedgerFencedException: Can't trim fenced ledger
	at org.apache.pulsar.broker.service.persistent.PersistentTopic$6.deleteLedgerFailed(PersistentTopic.java:1248)
	at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.lambda$asyncDelete$34(ManagedLedgerImpl.java:2737)
	at java.base/java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:863)
	at java.base/java.util.concurrent.CompletableFuture.uniWhenCompleteStage(CompletableFuture.java:887)
	at java.base/java.util.concurrent.CompletableFuture.whenComplete(CompletableFuture.java:2325)
	at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncDelete(ManagedLedgerImpl.java:2734)
	at org.apache.pulsar.broker.service.persistent.PersistentTopic.lambda$delete$33(PersistentTopic.java:1220)
	at java.base/java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:863)

@eolivelli
Copy link
Contributor Author

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.

@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Sep 29, 2022
@merlimat merlimat added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Sep 29, 2022
@merlimat merlimat merged commit f0b6348 into apache:master Sep 29, 2022
@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Sep 29, 2022
@dlg99 dlg99 mentioned this pull request Oct 4, 2022
Merged
5 tasks
@dlg99 dlg99 mentioned this pull request Oct 25, 2022
Merged
5 tasks
dlg99 added a commit to datastax/pulsar that referenced this pull request Oct 31, 2022
@dlg99
[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic Dele…
059f444 
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderzc coderzc coderzc approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@lhotari lhotari lhotari approved these changes

Assignees

@eolivelli eolivelli

Labels
doc-label-missing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eolivelli @mattisonchao @cbornet @lhotari @nicoloboschi @coderzc @merlimat