eric-maynard has posted comments on this change. Change subject: KUDU-1673 [java client] more informative kudu-spark write error ......................................................................
Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/5725/5//COMMIT_MSG Commit Message: Line 7: KUDU-1673 [java client] more informative kudu-spark write error > can you add a paragraph to the commit message which explains a bit more wha Done PS5, Line 11: an exception which contained all of the errors encountered. : : Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5 > please remove this redundant stuff from the message (seems like it's left o Done http://gerrit.cloudera.org:8080/#/c/5725/6//COMMIT_MSG Commit Message: PS6, Line 10: It would be better to throw : an exception which contained all of the errors encountered. > I may be misunderstanding - but I don't see the motivation for this change. >From the Jira: "When a KuduContext write fails it throws a RuntimeException >with some concatenated sampled error messages. This is somewhat helpful for >visual debugging, but for more complete debugging/reporting we could also do >with having an exception we could catch that contained the actual error >objects." I can't speak for the "motivation" of the Jira, really, but to say >that it was opened by a clouderan several months ago. In my mind the idea is to no longer lean on toString or the exception message but to make a more catchable exception for debugging. Indeed, though, I did provide a small getSample method in WriteException which can collect a string representation of a few sample errors. Putting this sample in the toString is a good idea. http://gerrit.cloudera.org:8080/#/c/5725/5/java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java File java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java: Line 1: // Licensed to the Apache Software Foundation (ASF) under one > style: add license Done Line 6: // "License"); you may not use this file except in compliance > I don't think we want to be in the business of throwing unchecked exception I agree that a checked exception would be preferable. However, we are currently using this exception in place of a RuntimeException. i.e. we have already been in that business, but if you want to get out that's great. Should we extend KuduException? Or is there another interface to implement? Line 22: import org.apache.kudu.annotations.InterfaceAudience; > hopefully this could be package-private, or annotated as private I agree, indeed initially it was package-private, but we'd have to change the location from org.apache.kudu.client if that is to work. I'd like to hear your thoughts on where we could place it. Currently we're using it in the scala KuduContext only. Line 23: import org.apache.kudu.annotations.InterfaceStability; > isn't this implicit? Done Line 46: /** > style: spacing before (, after ) Done http://gerrit.cloudera.org:8080/#/c/5725/6/java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java File java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java: Line 23: import org.apache.kudu.annotations.InterfaceStability; > Add visibility/stability annotations. Done Line 28: > All of the block comments in this file are in the scala style, not the java Done PS6, Line 36: > no capital here and below Done -- To view, visit http://gerrit.cloudera.org:8080/5725 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: eric-maynard <emayn...@cloudera.com> Gerrit-Reviewer: Chris George <chris.geo...@rms.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: eric-maynard <emayn...@cloudera.com> Gerrit-HasComments: Yes
