[fix][broker] PulsarLedgerManager to pass correct error code to BK client #16857

dlg99 · 2022-07-28T22:18:02Z

Motivation

In some situations it is possible to encounter case when deletion of a ManagedLedger deals with cases of already deleted bookie ledgers.
Such cases currently handled as errors even though they are safe to ignore.
Currently, it is impossible to handle these cases because PulsarManagedLedger returns error that's not mappable into the BK error code end the end user ends up with obscure UnexpectedConditionException (error code -999) that cannot be distinguished from ledger already deleted case.

Modifications

Made PulsarManagedLedger return BK error where possible so BK client has a chance to handle/return correct error.
Ignored NoSuchLedgerExistsOnMetadataServerException on delete as it is safe there

This takes over #16664

Verifying this change

This change added tests and can be verified as follows:

Added unit tests

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

If yes was chosen, please highlight the changes

Nothing that I can think of.

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)

…eletion of already deleted ledger

eolivelli

Overall lgtm

I have left two minor comments

eolivelli · 2022-07-29T06:16:39Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java

+
        CompletableFuture<Versioned<LedgerMetadata>> future = store.put(getLedgerPath(ledgerId), data, Optional.of(-1L))
-                .thenApply(stat -> new Versioned(metadata, new LongVersion(stat.getVersion())));
+                .thenApply(stat -> {


Nit:
We can replace this with thenAccept
or the both of the two calls (apply and exceptionally) with whenComplete

+1 for simplifying the code.

eolivelli · 2022-07-29T06:17:42Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java


+    private static Throwable mapToBkException(Throwable ex) {
+        if (ex instanceof MetadataStoreException.NotFoundException) {
+            return BKException.create(BKException.Code.NoSuchLedgerExistsOnMetadataServerException);


What about calling initCause and add the original error to the chain?

eolivelli · 2022-07-29T06:18:35Z

@codelipenghui @BewareMyPower I think that we should include this in 2.10

BewareMyPower · 2022-07-29T06:41:54Z

@eolivelli You can add the release/2.10.x label. Currently there is no 2.10.x release in progress.

hangc0276

Overall LGTM.

hangc0276 · 2022-07-29T14:49:04Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java

+
        CompletableFuture<Versioned<LedgerMetadata>> future = store.put(getLedgerPath(ledgerId), data, Optional.of(-1L))
-                .thenApply(stat -> new Versioned(metadata, new LongVersion(stat.getVersion())));
+                .thenApply(stat -> {


+1 for simplifying the code.

mattisonchao

LGTM left some minor concerns.

mattisonchao · 2022-07-30T14:53:54Z

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

+                                log.info("Ledger {} does not exist, ignoring", cursor.cursorsLedgerId);
+                                return null;
+                            }
+                            throw new CompletionException(ex);


If the exception is CompletionException we don't need to wrap it again. please see FutureUtil#wrapToCompletionException

mattisonchao · 2022-07-30T15:05:39Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java

-            log.error("Failed to create ledger {}: {}", ledgerId, ex.getMessage());
-            return null;
-        });
+        CompletableFuture<Versioned<LedgerMetadata>> promise = new CompletableFuture<>();


Just a small concern:
I prefer the original chain-style CompletableFuture. it has two benefits than current:

If we use a chain-style future, we don't need to create a separate CompletableFuture.

In the current approach, it has risks cause the future can not complete yet. for example, throw an exception before we complete the promise. (But this method looks fine.

Please let me know if you have any good suggestions. thanks a lot!

yes, agree. Why create a new future here?

eolivelli

Lgtm

RobertIndie

Overall looks good to me.

RobertIndie · 2022-08-01T03:42:49Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerBkEnsemblesTests.java

+        try {
+            bookKeeper.deleteLedger(firstLedgerToDelete);
+        } catch (BKException.BKNoSuchLedgerExistsOnMetadataServerException bke) {
+            // pass
+        }


Suggested change

try {

bookKeeper.deleteLedger(firstLedgerToDelete);

} catch (BKException.BKNoSuchLedgerExistsOnMetadataServerException bke) {

// pass

}

try {

bookKeeper.deleteLedger(firstLedgerToDelete);

fail();

} catch (BKException.BKNoSuchLedgerExistsOnMetadataServerException bke) {

// pass

}

Please use fail() here.

Technoboy-

Merge this patch first, let it go 2.11, and then we can fix it with @RobertIndie comment given in the test of ml.

…ient (#16857) (cherry picked from commit 2e8bd3d)

…ient (apache#16857) (cherry picked from commit 2e8bd3d) (cherry picked from commit 796dd62)

dlg99 requested review from eolivelli and merlimat July 28, 2022 22:18

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

dlg99 mentioned this pull request Jul 28, 2022

[fix][broker] PulsarLedgerManager to pass correct error code to BK client #16664

Closed

4 tasks

PulsarLedgerManager to pass correct error code to BK client; handle d…

bd7cbbe

…eletion of already deleted ledger

dlg99 force-pushed the metadata-rc-bk-take2 branch from 58bb354 to bd7cbbe Compare July 28, 2022 22:32

eolivelli approved these changes Jul 29, 2022

View reviewed changes

eolivelli assigned dlg99 Jul 29, 2022

eolivelli modified the milestones: 2.12.0, 2.11.0 Jul 29, 2022

eolivelli added type/bug The PR fixed a bug or issue reported a bug area/broker release/2.10.2 labels Jul 29, 2022

eolivelli requested review from BewareMyPower and codelipenghui July 29, 2022 08:18

codelipenghui approved these changes Jul 29, 2022

View reviewed changes

hangc0276 approved these changes Jul 29, 2022

View reviewed changes

CR feedback

76a9bc5

mattisonchao approved these changes Jul 30, 2022

View reviewed changes

eolivelli approved these changes Jul 30, 2022

View reviewed changes

RobertIndie reviewed Aug 1, 2022

View reviewed changes

Technoboy- approved these changes Aug 3, 2022

View reviewed changes

Technoboy- merged commit 2e8bd3d into apache:master Aug 3, 2022

codelipenghui pushed a commit that referenced this pull request Aug 8, 2022

[fix][broker] PulsarLedgerManager to pass correct error code to BK cl…

796dd62

…ient (#16857) (cherry picked from commit 2e8bd3d)

codelipenghui added the cherry-picked/branch-2.10 label Aug 8, 2022

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2022

[fix][broker] PulsarLedgerManager to pass correct error code to BK cl…

d68dd3b

…ient (apache#16857) (cherry picked from commit 2e8bd3d) (cherry picked from commit 796dd62)

codelipenghui mentioned this pull request Nov 27, 2022

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

Merged

1 task

[fix][broker] PulsarLedgerManager to pass correct error code to BK client #16857

[fix][broker] PulsarLedgerManager to pass correct error code to BK client #16857

Uh oh!

Conversation

dlg99 commented Jul 28, 2022 • edited by hangc0276 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Jul 29, 2022

Uh oh!

BewareMyPower commented Jul 29, 2022

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattisonchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

RobertIndie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Technoboy- left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dlg99 commented Jul 28, 2022 •

edited by hangc0276

Loading