Dan Burkert has posted comments on this change. Change subject: RFC [java client] Redo how we manage exceptions ......................................................................
Patch Set 1: (5 comments) Overall I like this change, but I don't think it goes far enough. I'm a fan of changing all of our public interfaces to throw KuduException instead of Exception. According to https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.21 this wouldn't be a breaking change. However, I think this is already a breaking change in a different way, since we are removing public exception classes, so IMO it's worth going the whole way. As I mentioned in a few places, I don't have a clear picture on what Recoverable vs. NonRecoverable means for client applications. Should applications be catching specific instances of one or the other? Is Recoverable exception ever thrown through a public interface? The Kudu RPC system has a somewhat convoluted error system in that the RPC system has it's own error `ErrorStatusPB`, there is a generic application error type (`AppStatusPB`), and both the master and tserver have their own error type which wraps `AppStatusPB` (`MasterErrorPB` and `TabletServerErrorPB`). It's maybe outside the scope of this change, but it's not clear how those compose. And how are they being translated into KuduException? http://gerrit.cloudera.org:8080/#/c/3055/1//COMMIT_MSG Commit Message: Line 20: \-> RecoverableException Is the distinction between Recoverable and NonRecoverable important from a client application perspective? http://gerrit.cloudera.org:8080/#/c/3055/1/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: public abstract class KuduException extends RuntimeException { > It's an asynchbase legacy. I'm a big +1 to making KuduException a subclass of IOException. It makes it possible to throw a KuduException from Closeable#close http://gerrit.cloudera.org:8080/#/c/3055/1/java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java File java/kudu-client/src/main/java/org/kududb/client/NonRecoverableException.java: Line 34: public class NonRecoverableException extends KuduException { I think this class could use more javadoc color. Specifically, it should be documented from the perspective of an application developer. It *is* potentially recoverable for the application, right? It also seems like there may be some confusion between non-recoverable for a single connection vs. non-recoverable at all. http://gerrit.cloudera.org:8080/#/c/3055/1/java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java File java/kudu-client/src/main/java/org/kududb/client/RecoverableException.java: Line 41: public class RecoverableException extends KuduException { Same here WRT more docs for application developers. Would we ever throw one of these through a public interface? Seems like we shouldn't if it's recoverable. http://gerrit.cloudera.org:8080/#/c/3055/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java File java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java: Line 82: fail("Shoulnd't be able to alter a table that doesn't exist"); Shouldn't -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
