Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 5:

(16 comments)

> (16 comments)
 > 
 > 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.

http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

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.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java:

Line 843:         Status statusCorruption = Status.Corruption("Scan RPC 
response was for scanner"
> I think we typically reserve "corruption" for scrambled data and equivalent
Done


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

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
Done


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.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
File 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java:

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.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/KuduClient.java:

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


Line 107:       } catch (Exception ex) {
> Isn't it only d.join() that needs to be surrounded in the try/catch?
isAlterTableDone too.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:

> 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.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/KuduSession.java:

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


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


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
File 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java:

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


Line 53:   static NoLeaderMasterFoundException create(String msg, 
List<Exception> causes) {
> What value does this factory method add? I think all other exceptions are c
Done


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java:

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 
RowResult.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/Status.java
File java/kudu-client/src/main/java/org/kududb/client/Status.java:

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.


http://gerrit.cloudera.org:8080/#/c/3055/5/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:

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 http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to