You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/07/24 08:33:36 UTC
[GitHub] [bookkeeper] eolivelli opened a new pull request #2384: Track ZooKeeper errors as causes of ZKException
eolivelli opened a new pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384
Descriptions of the changes in this PR:
### Motivation
Every time a problem with ZK arises you don't see it in the exception chain of the BKException (in this case ZKException) and you end up with errors like:
```
org.apache.bookkeeper.client.BKException$ZKException: Error while using ZooKeeper
at org.apache.bookkeeper.client.SyncCallbackUtils.finish(SyncCallbackUtils.java:83)
at org.apache.bookkeeper.client.SyncCallbackUtils$SyncAddCallback.addComplete(SyncCallbackUtils.java:251)
at org.apache.bookkeeper.client.AsyncCallback$AddCallback.addCompleteWithLatency(AsyncCallback.java:91)
at org.apache.bookkeeper.client.PendingAddOp.submitCallback(PendingAddOp.java:430)
at org.apache.bookkeeper.client.LedgerHandle.errorOutPendingAdds(LedgerHandle.java:1784)
at org.apache.bookkeeper.client.LedgerHandle$5.safeRun(LedgerHandle.java:574)
```
### Changes
Add a "cause" to every ZKException.
### Notes
There are very few places that cannot be fixed because they are still using the old callback based mechanism without CompletableFuture. Those points are not changed in order to make the patch simple but still useful.
> ---
> In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
> checks for pull requests. A pull request can only be merged when it passes precommit checks.
>
> ---
> Be sure to do all of the following to help us incorporate your contribution
> quickly and easily:
>
> If this PR is a BookKeeper Proposal (BP):
>
> - [ ] Make sure the PR title is formatted like:
> `<BP-#>: Description of bookkeeper proposal`
> `e.g. BP-1: 64 bits ledger is support`
> - [ ] Attach the master issue link in the description of this PR.
> - [ ] Attach the google doc link if the BP is written in Google Doc.
>
> Otherwise:
>
> - [ ] Make sure the PR title is formatted like:
> `<Issue #>: Description of pull request`
> `e.g. Issue 123: Description ...`
> - [ ] Make sure tests pass via `mvn clean apache-rat:check install spotbugs:check`.
> - [ ] Replace `<Issue #>` in the title with the actual Issue number.
>
> ---
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2384: Track ZooKeeper errors as causes of ZKException
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384#discussion_r459972085
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
##########
@@ -459,7 +460,8 @@ public void processResult(int rc, String path, Object ctx, byte[] data, Stat sta
promise.complete(new Versioned<>(metadata, version));
} catch (Throwable t) {
LOG.error("Could not parse ledger metadata for ledger: {}", ledgerId, t);
- promise.completeExceptionally(new BKException.ZKException());
+ promise.completeExceptionally(new BKException.ZKException(
+ new Exception("Could not parse ledger metadata for ledger: "+ledgerRootPath, t).fillInStackTrace()));
Review comment:
done, thanks
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
##########
@@ -444,12 +444,13 @@ public void processResult(int rc, String path, Object ctx, byte[] data, Stat sta
if (rc != KeeperException.Code.OK.intValue()) {
LOG.error("Could not read metadata for ledger: " + ledgerId,
KeeperException.create(KeeperException.Code.get(rc), path));
- promise.completeExceptionally(new BKException.ZKException());
+ promise.completeExceptionally(new BKException.ZKException(KeeperException.create(Code.get(rc), path)));
return;
}
if (stat == null) {
LOG.error("Could not parse ledger metadata for ledger: {}. Stat object is null", ledgerId);
- promise.completeExceptionally(new BKException.ZKException());
+ promise.completeExceptionally(new BKException.ZKException(
+ new Exception("Could not parse ledger metadata for ledger: "+ledgerId+" . Stat object is null").fillInStackTrace()));
Review comment:
done, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] mino181295 commented on a change in pull request #2384: Track ZooKeeper errors as causes of ZKException
Posted by GitBox <gi...@apache.org>.
mino181295 commented on a change in pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384#discussion_r459935027
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
##########
@@ -444,12 +444,13 @@ public void processResult(int rc, String path, Object ctx, byte[] data, Stat sta
if (rc != KeeperException.Code.OK.intValue()) {
LOG.error("Could not read metadata for ledger: " + ledgerId,
KeeperException.create(KeeperException.Code.get(rc), path));
- promise.completeExceptionally(new BKException.ZKException());
+ promise.completeExceptionally(new BKException.ZKException(KeeperException.create(Code.get(rc), path)));
return;
}
if (stat == null) {
LOG.error("Could not parse ledger metadata for ledger: {}. Stat object is null", ledgerId);
- promise.completeExceptionally(new BKException.ZKException());
+ promise.completeExceptionally(new BKException.ZKException(
+ new Exception("Could not parse ledger metadata for ledger: "+ledgerId+" . Stat object is null").fillInStackTrace()));
Review comment:
Checkstyle?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
##########
@@ -459,7 +460,8 @@ public void processResult(int rc, String path, Object ctx, byte[] data, Stat sta
promise.complete(new Versioned<>(metadata, version));
} catch (Throwable t) {
LOG.error("Could not parse ledger metadata for ledger: {}", ledgerId, t);
- promise.completeExceptionally(new BKException.ZKException());
+ promise.completeExceptionally(new BKException.ZKException(
+ new Exception("Could not parse ledger metadata for ledger: "+ledgerRootPath, t).fillInStackTrace()));
Review comment:
Checkstyle?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2384: Track ZooKeeper errors as causes of ZKException
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384#issuecomment-663410445
@Ghatage @mino181295 would you pleas take a look ?
@jiazhai @rdhabalia this patch is good to be ported to 4.10 branch, it is low risk
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] mino181295 commented on pull request #2384: Track ZooKeeper errors as causes of ZKException
Posted by GitBox <gi...@apache.org>.
mino181295 commented on pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384#issuecomment-663520586
Last check failing on AbstractZkLedgerManager.java _line too long_
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli edited a comment on pull request #2384: Track ZooKeeper errors as causes of ZKException
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384#issuecomment-663410445
@Ghatage @mino181295 would you pleas take a look ?
@jiazhai @rdhabalia this patch is good to be ported to 4.11 and to 4.10, it is low risk
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2384: Track ZooKeeper errors as causes of ZKException
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2384:
URL: https://github.com/apache/bookkeeper/pull/2384#issuecomment-663472567
@mino181295 I have fixed checkstyle errors. PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org