You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "eric-maynard (Code Review)" <ge...@cloudera.org> on 2017/01/17 16:06:03 UTC
[kudu-CR] Initial commit
eric-maynard has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5725
Change subject: Initial commit
......................................................................
Initial commit
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
1 file changed, 52 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/1
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5725
to look at the new patch set (#4).
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
KUDU-1673 [java client] more informative kudu-spark write error
Change-Id: I935b5d1c61a8430ec48b90071374160505224c69
Initial commit
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/4
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5725
to look at the new patch set (#3).
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
KUDU-1673 [java client] more informative kudu-spark write error
Change-Id: I935b5d1c61a8430ec48b90071374160505224c69
Initial commit
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 54 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/3
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
Patch Set 6:
eric-maynard: oops! When writing that review I mistakenly thought Todd had written this, so I didn't add a lot of color about why those changes were necessary, let me know if anything doesn't make sense. Thanks for the patch!
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Chris George <ch...@rms.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
Gerrit-HasComments: No
[kudu-CR] Added support for WriteException
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
eric-maynard has abandoned this change.
Change subject: Added support for WriteException
......................................................................
Abandoned
Rolled into
https://gerrit.cloudera.org/#/c/5725/
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "Chris George (Code Review)" <ge...@cloudera.org>.
Chris George has posted comments on this change.
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
Patch Set 7:
are there any issues with having the write exception serialized back to the spark master when running in a cluster? It may or may not be a problem... but should be tested in a cluster scenario so there isn't some kind of serialization issue
--
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 <em...@cloudera.com>
Gerrit-Reviewer: Chris George <ch...@rms.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
eric-maynard has posted comments on this change.
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
Patch Set 5:
thanks jenkins <3
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
Patch Set 6:
(4 comments)
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. Will WriteException's toString or message contain a sample of the row errors?
Why is it better to throw an exception which contains all of the errors? Callers will have to downcast or catch the specific error manually and then to-string those, I don't see that being a likely pattern in user applications.
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: public class WriteException extends RuntimeException {
Add visibility/stability annotations.
Line 28: * Default constructor.
All of the block comments in this file are in the scala style, not the java style.
PS6, Line 36: A
no capital here and below
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Chris George <ch...@rms.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
Patch Set 5:
(7 comments)
this would need appropriate tests before it can be committed
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 what the change is? It's annoying to have to go check JIRA for details on the bug (even though it may feel redundant to "duplicate" the summary from the JIRA)
PS5, Line 11: Initial commit
:
: Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
please remove this redundant stuff from the message (seems like it's left over from a squash)
This review seems to be using the 'I663ddd' change id, so leave that one in palce and delete the I935b one
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: package org.apache.kudu.client;
style: add license
Line 6: public class WriteException extends RuntimeException {
I don't think we want to be in the business of throwing unchecked exceptions.
This would also need an interface annotation and appropriate javadoc.
Line 22: public WriteException(RowError[] errors) {
hopefully this could be package-private, or annotated as private
Line 23: super();
isn't this implicit?
Line 46: for(int i = 0; i < n && i < errorList.size(); i++){
style: spacing before (, after )
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5725
to look at the new patch set (#5).
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
KUDU-1673 [java client] more informative kudu-spark write error
Change-Id: I935b5d1c61a8430ec48b90071374160505224c69
Initial commit
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/5
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Added support for WriteException
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
eric-maynard has uploaded a new patch set (#2).
Change subject: Added support for WriteException
......................................................................
Added support for WriteException
Change-Id: I935b5d1c61a8430ec48b90071374160505224c69
Initial commit
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 54 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/2
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
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 <em...@cloudera.com>
Gerrit-Reviewer: Chris George <ch...@rms.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5725
to look at the new patch set (#7).
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
KUDU-1673 [java client] more informative kudu-spark write error
When a KuduContext write fails it currently throws a RuntimeException
carrying a message with 5 sample errors. It would be better to throw
an exception which contained all of the errors encountered.
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 74 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/7
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Chris George <ch...@rms.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>
[kudu-CR] Added support for WriteException
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
eric-maynard has restored this change.
Change subject: Added support for WriteException
......................................................................
Restored
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: restore
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1673 [java client] more informative kudu-spark write error
Posted by "eric-maynard (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5725
to look at the new patch set (#6).
Change subject: KUDU-1673 [java client] more informative kudu-spark write error
......................................................................
KUDU-1673 [java client] more informative kudu-spark write error
When a KuduContext write fails it currently throws a RuntimeException
carrying a message with 5 sample errors. It would be better to throw
an exception which contained all of the errors encountered.
Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
---
A java/kudu-client/src/main/java/org/apache/kudu/client/WriteException.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 70 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/5725/6
--
To view, visit http://gerrit.cloudera.org:8080/5725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I663dddbf1b01f9c3d90f92dd484fc1a0018243c5
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: eric-maynard <em...@cloudera.com>
Gerrit-Reviewer: Chris George <ch...@rms.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: eric-maynard <em...@cloudera.com>