Adar Dembo 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?
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" 
error so that in the event of a timeout, we can provide that to the user too.

Line 1280:           return new NonRecoverableException(status);
Ugh. Can we use an errback instead of using the callback for exceptions too?
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. 
Maybe IllegalState or InvalidArgument here?
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 sure 
they have a @throws.

Line 536:                 Status.ConfigurationError("MANUAL_FLUSH is enabled 
but the buffer is too big");
Maybe IllegalState instead?

Line 127:           Status statusConfigurationError = Status.ConfigurationError(
Why not NotFound?
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?
File java/kudu-client/src/main/java/org/kududb/client/

Why RuntimeExceptions from this file and not some kind of KuduException?
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.

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 
constructed directly; can we do the same here?

I think it'd make more sense that way, because then the caller is responsible 
for constructing the Status, and the caller is best positioned to know exactly 
what kind of Status to use.
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 throw 
a checked exception?
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 
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.

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