[kudu-CR] [java client] Redo how we manage exceptions

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

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


Patch Set 12: Code-Review+2

-- 
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: 12
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 12:

Build Started http://104.196.14.100/job/kudu-gerrit/2583/

-- 
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: 12
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3055

to look at the new patch set (#12).

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

[java client] Redo how we manage exceptions

Right now the exceptions are hard to handle in the Java client. They're all
generic and you need to do a lot of introspection. For example, if you try
to create a table that already exists, you need to start searching the
exception's message to know if it's that or some other problem that gave
you the error.

With this patch we now only one main kind of public exception: KuduException.
We still have Recoverable/NonRecoverableException but those are now
package-private and only used internally. PleaseThrottleException is kept public
for the async API.

KuduException has a new field, `status`, which is your regular Kudu Status
object. Wherever we can we try to recreate the Status objects that are sent
to us from the servers, else we create our own. Now for the example above we
can just query the exception's status with `isNotFound()`.

The sync APIs is also modified to only throw KuduExceptions instead of plain
Exceptions.

Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/ConnectionResetException.java
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
D java/kudu-client/src/main/java/org/kududb/client/InvalidResponseException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/KuduException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
D java/kudu-client/src/main/java/org/kududb/client/KuduServerException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/MasterErrorException.java
M 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
M java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/kududb/client/Status.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
D 
java/kudu-client/src/main/java/org/kududb/client/TabletServerErrorException.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/kududb/client/TestTimeouts.java
26 files changed, 406 insertions(+), 520 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/3055/12
-- 
To view, visit http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 12
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 

[kudu-CR] [java client] Redo how we manage exceptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 11:

Build Started http://104.196.14.100/job/kudu-gerrit/2582/

-- 
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: 11
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 10:

Build Started http://104.196.14.100/job/kudu-gerrit/2576/

-- 
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: 10
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3055

to look at the new patch set (#10).

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

[java client] Redo how we manage exceptions

Right now the exceptions are hard to handle in the Java client. They're all
generic and you need to do a lot of introspection. For example, if you try
to create a table that already exists, you need to start searching the
exception's message to know if it's that or some other problem that gave
you the error.

With this patch we now only one main kind of public exception: KuduException.
We still have Recoverable/NonRecoverableException but those are now
package-private and only used internally. PleaseThrottleException is kept public
for the async API.

KuduException has a new field, `status`, which is your regular Kudu Status
object. Wherever we can we try to recreate the Status objects that are sent
to us from the servers, else we create our own. Now for the example above we
can just query the exception's status with `isNotFound()`.

The sync APIs is also modified to only throw KuduExceptions instead of plain
Exceptions.

Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/ConnectionResetException.java
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
D java/kudu-client/src/main/java/org/kududb/client/InvalidResponseException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/KuduException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
D java/kudu-client/src/main/java/org/kududb/client/KuduServerException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/MasterErrorException.java
M 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
M java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/kududb/client/Status.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
D 
java/kudu-client/src/main/java/org/kududb/client/TabletServerErrorException.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/kududb/client/TestTimeouts.java
26 files changed, 393 insertions(+), 505 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/3055/10
-- 
To view, visit http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 10
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 

[kudu-CR] [java client] Redo how we manage exceptions

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

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


Patch Set 8:

(1 comment)

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

Line 99:   public boolean isAlterTableDone(String name) throws KuduException, 
InterruptedException {
> Ugh yeah looks like in other places where we call Deferred.join we just pro
I think KuduException#transformException should specifically check for 
InterruptedException, and if it finds it, set the threads interrupt flag.


-- 
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: 8
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

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3055

to look at the new patch set (#9).

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

[java client] Redo how we manage exceptions

Right now the exceptions are hard to handle in the Java client. They're all
generic and you need to do a lot of introspection. For example, if you try
to create a table that already exists, you need to start searching the
exception's message to know if it's that or some other problem that gave
you the error.

With this patch we now only one main kind of public exception: KuduException.
We still have Recoverable/NonRecoverableException but those are now
package-private and only used internally. PleaseThrottleException is kept public
for the async API.

KuduException has a new field, `status`, which is your regular Kudu Status
object. Wherever we can we try to recreate the Status objects that are sent
to us from the servers, else we create our own. Now for the example above we
can just query the exception's status with `isNotFound()`.

The sync APIs is also modified to only throw KuduExceptions instead of plain
Exceptions.

Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/ConnectionResetException.java
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
D java/kudu-client/src/main/java/org/kududb/client/InvalidResponseException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/KuduException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
D java/kudu-client/src/main/java/org/kududb/client/KuduServerException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/MasterErrorException.java
M 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
M java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/kududb/client/Status.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
D 
java/kudu-client/src/main/java/org/kududb/client/TabletServerErrorException.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/kududb/client/TestTimeouts.java
26 files changed, 401 insertions(+), 520 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/3055/9
-- 
To view, visit http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 9
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 

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 8:

(3 comments)

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

Line 99:   public boolean isAlterTableDone(String name) throws KuduException, 
InterruptedException {
> It's not clear to me why this one throws Interrupted but not any of the oth
Ugh yeah looks like in other places where we call Deferred.join we just process 
everything as Exception.


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

Line 37:  * sse if you're using the non-async API, such as {@link KuduSession} 
instead of
> s/sse/see
Done


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

Line 26: public class NonCoveredRangeException extends NonRecoverableException {
> Can this be default (package private) visibility?
Good catch.


-- 
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: 8
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

[kudu-CR] [java client] Redo how we manage exceptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/2552/

-- 
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: 9
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

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


Patch Set 8:

(3 comments)

Looking great overall.

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

Line 99:   public boolean isAlterTableDone(String name) throws KuduException, 
InterruptedException {
It's not clear to me why this one throws Interrupted but not any of the other 
methods.  Seems like either all or none should.


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

Line 37:  * sse if you're using the non-async API, such as {@link KuduSession} 
instead of
s/sse/see


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

Line 26: public class NonCoveredRangeException extends NonRecoverableException {
Can this be default (package private) visibility?


-- 
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: 8
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

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 8:

> hm, what's the status on this. Still waiting on Dan?

Yeah I told Dan that helping out Adar first with reviews was better since he's 
basing his add/remove partitions on top of Adar's patches.

-- 
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: 8
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 8: Code-Review+1

Will leave the +2ing for Dan, since he had some comments originally.

-- 
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: 8
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: No

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 7:

(2 comments)

> (3 comments)
 > 
 > Only a few nits left.
 > 
 > When you're done, could you make a pass over the client-consuming
 > modules in the java subdirectory and update their exception
 > handling, if need be? In a separate patch, of course.

Sure can do.

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

Line 25: @InterfaceAudience.Public
> Hmm, shouldn't this be Private?
Done


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

Line 55: int numRows, Slice bs, Slice indirectBs) {
> Nit: indentation here looks off.
last minute changes...


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

[kudu-CR] [java client] Redo how we manage exceptions

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 7:

(3 comments)

Only a few nits left.

When you're done, could you make a pass over the client-consuming modules in 
the java subdirectory and update their exception handling, if need be? In a 
separate patch, of course.

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

Line 25: @InterfaceAudience.Public
Hmm, shouldn't this be Private?


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

Line 55: int numRows, Slice bs, Slice indirectBs) {
Nit: indentation here looks off.


PS7, Line 66: Schema 
schema,
: 
WireProtocol.RowwiseRowBlockPB data,
: final 
CallResponse callResponse)
Nit: here too.


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

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3055

to look at the new patch set (#7).

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

[java client] Redo how we manage exceptions

Right now the exceptions are hard to handle in the Java client. They're all
generic and you need to do a lot of introspection. For example, if you try
to create a table that already exists, you need to start searching the
exception's message to know if it's that or some other problem that gave
you the error.

With this patch we now only one main kind of public exception: KuduException.
We still have Recoverable/NonRecoverableException but those are now
package-private and only used internally. PleaseThrottleException is kept public
for the async API.

KuduException has a new field, `status`, which is your regular Kudu Status
object. Wherever we can we try to recreate the Status objects that are sent
to us from the servers, else we create our own. Now for the example above we
can just query the exception's status with `isNotFound()`.

The sync APIs is also modified to only throw KuduExceptions instead of plain
Exceptions.

Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/ConnectionResetException.java
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
D java/kudu-client/src/main/java/org/kududb/client/InvalidResponseException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/KuduException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
D java/kudu-client/src/main/java/org/kududb/client/KuduServerException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/MasterErrorException.java
M 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
M java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/kududb/client/Status.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
D 
java/kudu-client/src/main/java/org/kududb/client/TabletServerErrorException.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/kududb/client/TestTimeouts.java
26 files changed, 387 insertions(+), 503 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/3055/7
-- 
To view, visit http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 7
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 

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 5:

(1 comment)

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");
> Yeah, I find throwing from a constructor to be generally weird. And beyond 
Alright going with a factory method.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java client] Redo how we manage exceptions

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 5:

(2 comments)

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);
> Right I'm saying this is equivalent, the cause is what is prompting us to r
Oh, I forgot there's a Status for each exception in the chain. In that case, 
yes, I think this is fine, as there's a programmatic representation for each 
cause.


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");
> I'm not fully understanding your concern with having this here. Is it just 
Yeah, I find throwing from a constructor to be generally weird. And beyond 
that, throwing a checked exception from a constructor is even weirder, and I 
think this should be a KuduException because it's thrown in response to bad 
on-the-wire data.

An init() method is okay too, it's just more work for the caller than a factory 
method (i.e. two calls to make instead of one).


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3055

to look at the new patch set (#6).

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

[java client] Redo how we manage exceptions

Right now the exceptions are hard to handle in the Java client. They're all
generic and you need to do a lot of introspection. For example, if you try
to create a table that already exists, you need to start searching the
exception's message to know if it's that or some other problem that gave
you the error.

With this patch we now only one main kind of public exception: KuduException.
We still have Recoverable/NonRecoverableException but those are now
package-private and only used internally. PleaseThrottleException is kept public
for the async API.

KuduException has a new field, `status`, which is your regular Kudu Status
object. Wherever we can we try to recreate the Status objects that are sent
to us from the servers, else we create our own. Now for the example above we
can just query the exception's status with `isNotFound()`.

The sync APIs is also modified to only throw KuduExceptions instead of plain
Exceptions.

Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/ConnectionResetException.java
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
D java/kudu-client/src/main/java/org/kududb/client/InvalidResponseException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/KuduException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
D java/kudu-client/src/main/java/org/kududb/client/KuduServerException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/MasterErrorException.java
M 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
M java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/kududb/client/Status.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
D 
java/kudu-client/src/main/java/org/kududb/client/TabletServerErrorException.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/kududb/client/TestTimeouts.java
26 files changed, 357 insertions(+), 485 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/3055/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
Gerrit-PatchSet: 6
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 

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 5:

(3 comments)

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);
> Yes and no. The cause does give the user more information, but if the seque
Right I'm saying this is equivalent, the cause is what is prompting us to 
retry. Here you'd get a NonRecoverable with a TimedOut status and a cause that 
might be TOO_BUSY.


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 107:   } catch (Exception ex) {
> Why? It's an async operation; why would it throw?
Huh right, why is it throwing... removed.


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");
> What if we added a factory method to build RowResultIterators, and performe
I'm not fully understanding your concern with having this here. Is it just that 
you'd rather not throw from a constructor? For that we could just have a 
"init()" method that'd be called right after the constructor.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

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

[kudu-CR] [java client] Redo how we manage exceptions

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 5:

(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?

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" 
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?


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. 
Maybe IllegalState or InvalidArgument here?


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


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?


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.


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


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?


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.


PS5, Line 99: DeferredGroupException.
We shouldn't mention this; it's not a public exception any more.


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


Line 53:   static NoLeaderMasterFoundException create(String msg, 
List 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.


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 throw 
a checked exception?


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 

[kudu-CR] [java client] Redo how we manage exceptions

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2333/

-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java client] Redo how we manage exceptions

[Jean-Daniel Cryans (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3055

to look at the new patch set (#5).

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

[java client] Redo how we manage exceptions

Right now the exceptions are hard to handle in the Java client. They're all
generic and you need to do a lot of introspection. For example, if you try
to create a table that already exists, you need to start searching the
exception's message to know if it's that or some other problem that gave
you the error.

With this patch we now only one main kind of public exception: KuduException.
We still have Recoverable/NonRecoverableException but those are now
package-private and only used internally. PleaseThrottleException is kept public
for the async API.

KuduException has a new field, `status`, which is your regular Kudu Status
object. Wherever we can we try to recreate the Status objects that are sent
to us from the servers, else we create our own. Now for the example above we
can just query the exception's status with `isNotFound()`.

The sync APIs is also modified to only throw KuduExceptions instead of plain
Exceptions.

Change-Id: Iba6e8a022d7a5391c3657cbdc9d3f06f951be048
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/ConnectionResetException.java
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
D java/kudu-client/src/main/java/org/kududb/client/InvalidResponseException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/KuduException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
D java/kudu-client/src/main/java/org/kududb/client/KuduServerException.java
M java/kudu-client/src/main/java/org/kududb/client/KuduSession.java
D java/kudu-client/src/main/java/org/kududb/client/MasterErrorException.java
M 
java/kudu-client/src/main/java/org/kududb/client/NoLeaderMasterFoundException.java
M java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/kududb/client/Status.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
D 
java/kudu-client/src/main/java/org/kududb/client/TabletServerErrorException.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
M java/kudu-client/src/test/java/org/kududb/client/TestTimeouts.java
26 files changed, 324 insertions(+), 447 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/3055/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3055
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
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