Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Redo how we manage exceptions

Patch Set 5:


 > Definitely make a pass over the Javadoc for:
 > 1. Adding @throws where necessary, and
 > 2. Updating public-facing docs to avoid mentioning private
 > exception classes.
 > Also, what value does the separation between RecoverableException
 > and NonRecoverableException bring, for internal client use that is?

So we easily know when we can and can not retry. In some cases we do use more 
specialized exceptions though.
File java/kudu-client/src/main/java/org/kududb/client/

Line 1029:     Status statusTimedOut = Status.TimedOut(message + request);
> Perhaps unrelated, but in the C++ client we try to preserve the last "real"
Isn't that "cause" below?

Line 1280:           return new NonRecoverableException(status);
> Ugh. Can we use an errback instead of using the callback for exceptions too
Doing this is really just the way to convert a callback chain to errback. You 
can't callback and errback at the same time.
File java/kudu-client/src/main/java/org/kududb/client/

Line 843:         Status statusCorruption = Status.Corruption("Scan RPC 
response was for scanner"
> I think we typically reserve "corruption" for scrambled data and equivalent
File java/kudu-client/src/main/java/org/kududb/client/

Line 491:   public Deferred<OperationResponse> apply(final Operation operation) 
throws KuduException {
> Update Javadoc. Make a pass over the other changed methods too, to make sur

Line 536:                 Status.ConfigurationError("MANUAL_FLUSH is enabled 
but the buffer is too big");
> Maybe IllegalState instead?
I'm not feeling strongly either way, so I'll go with IllegalState.

Line 127:           Status statusConfigurationError = Status.ConfigurationError(
> Why not NotFound?
Why not? :) I think NotFound is more reserved for data, whereas here the user 
passed a list of masters (as a configuration) that isn't good.
File java/kudu-client/src/main/java/org/kududb/client/

Line 92:    * @throws NonRecoverableException for any error returned by sending 
RPCs to the master
> Nope, NonRecoverableException isn't public.

Line 107:       } catch (Exception ex) {
> Isn't it only d.join() that needs to be surrounded in the try/catch?
isAlterTableDone too.
File java/kudu-client/src/main/java/org/kududb/client/

> Why RuntimeExceptions from this file and not some kind of KuduException?
As a user you can't expect those exceptions to happen and it's not reasonable 
to recover from them.
File java/kudu-client/src/main/java/org/kududb/client/

Line 52:    * Blocking call with a different behavior based on the flush mode. 
PleaseThrottleException is
> Should update this to avoid mentioning private exception types.

PS5, Line 99: DeferredGroupException.
> We shouldn't mention this; it's not a public exception any more.
And we transform it now anyways.

Line 42:    * Factory method that creates a NoLeaderException given a message 
and a list
> Nit: NoLeaderMasterFoundException

Line 53:   static NoLeaderMasterFoundException create(String msg, 
List<Exception> causes) {
> What value does this factory method add? I think all other exceptions are c
File java/kudu-client/src/main/java/org/kududb/client/

PS5, Line 71:       throw new RuntimeException("RowResult block has " + 
bs.length() +
            :           " bytes of data but expected " + expectedSize + " for " 
+ numRows + " rows");
> Maybe we can move this check into a method where it'd be more natural to th
Pretty sure if this is wrong that it'll go boom below when constructing the 
File java/kudu-client/src/main/java/org/kududb/client/

PS5, Line 26: Wraps {@link org.kududb.WireProtocol.AppStatusPB}.
            :  * See also {@code src/kudu/util/status.h} in the C++ codebase.
> Maybe this part of the comment should be moved so it's not in public-facing
I kinda like referencing the C++ code, but the first part I can remove.
File java/kudu-client/src/main/java/org/kududb/client/

Line 258:       Status statusIllegalState = Status.IllegalState(wtf);
> I think "wtf" style errors qualify for RuntimeException.
It won't really matter here though right? On the other side we wrap everything 
in KuduExceptions if it's not already a KuduException.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to