[bugfix] ManagedLedger: move to FENCED state in case of BadVersionException #17736

eolivelli · 2022-09-20T08:29:51Z

Motivation

It may happen that a ManagedLedger continues to work even in case of seeing a BadVersionException.
For instance background activities like trimming ledgers, or offloading will continue to work.
This is very dangerous as it leads to some kind of split brain (it is not actually a split brain): two brokers continue to process the metadata (and data) of the same ledger.

Modifications

Force moving to Fenced state the ManagedLedger in case of BadVersionException
Fail the "offload loop" in case of Fenced state
Better cleanup of the PersistentTopic

Verifying this change

I added tests that cover the changes

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

If the box was checked, please highlight the changes

Documentation

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)

Matching PR in forked repository

PR in forked repository: eolivelli#16

After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.

apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.

The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.

-->

eolivelli · 2022-09-20T09:02:10Z

I am adding test cases in order to have examples of the problems and to prevent regressions in the future

zymap

Great work! I didn't realize that we can leverage the fence state to let the topic reload :)

It is a great way to resolve the bad version

nicoloboschi

Great work

dlg99 · 2022-09-20T17:56:20Z

LGTM but it broke testTruncateCorruptDataLedger

  Error:  Tests run: 9, Failures: 1, Errors: 0, Skipped: 3, Time elapsed: 45.675 s <<< FAILURE! - in org.apache.pulsar.broker.service.BrokerBkEnsemblesTests
  Error:  testTruncateCorruptDataLedger(org.apache.pulsar.broker.service.BrokerBkEnsemblesTests)  Time elapsed: 3.13 s  <<< FAILURE!
  org.apache.pulsar.client.admin.PulsarAdminException$ServerSideErrorException: 
  
   --- An unexpected error occurred in the server ---
  
  Message: Can't trim fenced ledger
  
  Stacktrace:
  
  org.apache.bookkeeper.mledger.ManagedLedgerException$ManagedLedgerAlreadyClosedException: Can't trim fenced ledger

lhotari

Good work @eolivelli !

michaeljmarshall

LGTM

eolivelli · 2022-09-21T08:33:12Z

@dlg99 I have fixed the test.
The test was trying do something that cannot happen.
it was calling "ledgerManager.delete()" and then "truncate()".

I removed "delete", the test is still meaningful, because it tests that the truncation works in case of missing ledgers (due to concurrent execution or previous deletion)

codelipenghui · 2022-09-21T13:17:38Z

We already handled the BadVersion in this way before https://github.com/apache/pulsar/pull/17736/files#diff-f6a849bd8fdb782ef6c17a2e07a2c54c3dc7d1655c00ec3546d5f3b3fc61e970L1537

Looks good to me. Approved the PR

hangc0276

A good solution for handling the ZK BadVersion exception.

hangc0276 · 2022-09-21T13:39:59Z

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

@@ -3639,6 +3664,7 @@ private void checkManagedLedgerIsOpen() throws ManagedLedgerException {
    }

    synchronized void setFenced() {
+        log.info("{} Moving to Fenced state", name);


Do we need to change the log level to warn?

This is not a "problem".
it may happen, and we are handling it safely.
there is nothing that the sysadmin should be afraid of.

we should log "WARN" or "ERROR" when there is something bad, and you have to take extra care

hangc0276 · 2022-09-21T13:42:04Z

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

@@ -3842,12 +3868,21 @@ private void scheduleTimeoutTask() {
                    ? Math.max(config.getAddEntryTimeoutSeconds(), config.getReadEntryTimeoutSeconds())
                    : timeoutSec;
            this.timeoutTask = this.scheduledExecutor.scheduleAtFixedRate(safeRun(() -> {


Suggest change

safeRun(() -> { checkTimeouts(); })

to safeRun(this::checkTimeouts)

hangc0276 · 2022-09-21T13:43:26Z

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

+        final State state = STATE_UPDATER.get(this);
+        if (state == State.Closed
+                || state == State.Fenced) {
+            return;


Do we need to add a warn log?

there are already logs that say that we fenced or closed the topic.
I don't think it is worth to add something more

gaoran10 · 2022-09-22T02:28:33Z

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

                log.debug("[{}] Ignoring trimming request since the managed ledger was already closed", name);
                trimmerMutex.unlock();
                promise.completeExceptionally(new ManagedLedgerAlreadyClosedException("Can't trim closed ledger"));
                return;
            }
+            if (currentState == State.Fenced) {
+                log.debug("[{}] Ignoring trimming request since the managed ledger was already fenced", name);
+                trimmerMutex.unlock();


Maybe we don't need to unlock the trimmerMutex.

I am not sure.
I prefer to keep the ML in a clean status.
I did the same it works for a "closed" ML.
it is very like to being "Closed" in this point

Sorry, after checking the code, I think we should release the lock, or else the scheduled task will always try to get the lock per 100 milliseconds.

Jason918

LGTM

…17736) (cherry picked from commit 63d4cf2)

@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)

…17736) (cherry picked from commit 63d4cf2)

…est license header (#18640) 1. cherry-pick: #17736 problem fix 2. fix cherry-pick 17526 bugs https://github.com/poorbarcode/pulsar/blob/4db04f509451dc1f17b96009b2f5e268c1ea644b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2015 it should change to !createTopicFuture.isPresent() not createTopicFuture.isPresent() 3. and fix import 4. fix cherry-pick https://github.com/apache/pulsar/pull18283 testDeleteTopicAndSchemaForV1, throw exception messages error 5. run branch-2.9 test ### Motivation fix branch-2.9 test ### Documentation - [x] `doc-not-needed`  ### Matching PR in forked repository

…17736)

… race conditions - solution was introduced in apache#17526 - however, it was accidentially replaced with a call to the incorrect method signature in apache#17736

eolivelli requested review from codelipenghui, zymap, hangc0276, lhotari and merlimat September 20, 2022 08:29

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

eolivelli added type/bug The PR fixed a bug or issue reported a bug area/tieredstorage area/broker labels Sep 20, 2022

eolivelli self-assigned this Sep 20, 2022

eolivelli changed the title ~~[bugfix] ManagedLedger: move to FENCED state in case of BadVersionException (DRAFT)~~ [bugfix] ManagedLedger: move to FENCED state in case of BadVersionException Sep 20, 2022

eolivelli requested review from dlg99, michaeljmarshall and nicoloboschi September 20, 2022 11:36

zymap approved these changes Sep 20, 2022

View reviewed changes

nicoloboschi approved these changes Sep 20, 2022

View reviewed changes

dlg99 approved these changes Sep 20, 2022

View reviewed changes

ManagedLedger: move to FENCED state in case of BadVersionException

136839c

eolivelli force-pushed the fix/badversion-fence-ml branch from a2c39b8 to 136839c Compare September 20, 2022 19:30

lhotari approved these changes Sep 20, 2022

View reviewed changes

michaeljmarshall approved these changes Sep 20, 2022

View reviewed changes

eolivelli added this to the 2.12.0 milestone Sep 21, 2022

zymap added release/2.9.4 release/2.8.5 labels Sep 21, 2022

codelipenghui approved these changes Sep 21, 2022

View reviewed changes

hangc0276 approved these changes Sep 21, 2022

View reviewed changes

gaoran10 requested changes Sep 22, 2022

View reviewed changes

Merge branch 'master' into fix/badversion-fence-ml

9316448

gaoran10 approved these changes Sep 22, 2022

View reviewed changes

Jason918 approved these changes Sep 22, 2022

View reviewed changes

eolivelli added the ready-to-test label Sep 22, 2022

eolivelli merged commit 63d4cf2 into apache:master Sep 22, 2022

Jason918 added release/2.10.2 and removed release/2.10.3 labels Sep 26, 2022

Jason918 pushed a commit that referenced this pull request Sep 26, 2022

ManagedLedger: move to FENCED state in case of BadVersionException (#…

4c0b672

…17736) (cherry picked from commit 63d4cf2)

Jason918 added the cherry-picked/branch-2.10 label Sep 26, 2022

Jason918 mentioned this pull request Sep 26, 2022

[fix][broker] Unregister topic policy listener if managed ledger close failed #17652

Merged

This was referenced Sep 29, 2022

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

Merged

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

Merged

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 pushed a commit that referenced this pull request Nov 14, 2022

ManagedLedger: move to FENCED state in case of BadVersionException (#…

f9e9dcd

…17736) (cherry picked from commit 63d4cf2)

congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 14, 2022

congbobo184 pushed a commit that referenced this pull request Nov 26, 2022

ManagedLedger: move to FENCED state in case of BadVersionException (#…

9dd4149

…17736) (cherry picked from commit 63d4cf2)

congbobo184 pushed a commit that referenced this pull request Nov 26, 2022

[cherry-pick][branch-2.9] cherry-pick #17736 problem

52fd58a

This was referenced Nov 26, 2022

[cherry-pick][branch-2.9] cherry-pick #17736 problem and run test #18639

Closed

[cherry-pick][branch-2.9] cherry-pick #17736 problem and DnsResolverTest license header #18640

Merged

Technoboy- pushed a commit that referenced this pull request Feb 8, 2023

ManagedLedger: move to FENCED state in case of BadVersionException (#…

d0d142d

…17736)

Technoboy- added the cherry-picked/branch-2.11 label Feb 8, 2023

lhotari mentioned this pull request Jun 7, 2023

[fix][broker] Restore solution for certain topic unloading race conditions #20527

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] ManagedLedger: move to FENCED state in case of BadVersionException #17736

[bugfix] ManagedLedger: move to FENCED state in case of BadVersionException #17736

eolivelli commented Sep 20, 2022 •

edited

Loading

eolivelli commented Sep 20, 2022

zymap left a comment

nicoloboschi left a comment

dlg99 commented Sep 20, 2022

lhotari left a comment

michaeljmarshall left a comment

eolivelli commented Sep 21, 2022

codelipenghui commented Sep 21, 2022

hangc0276 left a comment

hangc0276 Sep 21, 2022

eolivelli Sep 22, 2022

hangc0276 Sep 21, 2022

hangc0276 Sep 21, 2022

eolivelli Sep 22, 2022

gaoran10 Sep 22, 2022

eolivelli Sep 22, 2022

gaoran10 Sep 22, 2022

Jason918 left a comment

[bugfix] ManagedLedger: move to FENCED state in case of BadVersionException #17736

[bugfix] ManagedLedger: move to FENCED state in case of BadVersionException #17736

Conversation

eolivelli commented Sep 20, 2022 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

eolivelli commented Sep 20, 2022

zymap left a comment

Choose a reason for hiding this comment

nicoloboschi left a comment

Choose a reason for hiding this comment

dlg99 commented Sep 20, 2022

lhotari left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

eolivelli commented Sep 21, 2022

codelipenghui commented Sep 21, 2022

hangc0276 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jason918 left a comment

Choose a reason for hiding this comment

eolivelli commented Sep 20, 2022 •

edited

Loading