Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17518 )
Change subject: [client] KUDU-3267 Improve error logging when writing to non-existent partition through kudu java client ...................................................................... Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@7 PS1, Line 7: client nit: java We used to post changelists like this under 'java' tag. http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@7 PS1, Line 7: Improve error logging when writing to non-existent : partition through kudu java client How about this shorter one: improve logging on writes to non-existent partitions http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@10 PS1, Line 10: throw nit: log ? http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@11 PS1, Line 11: the java client the Kudu Java http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@12 PS1, Line 12: the kudu client the client http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@13 PS1, Line 13: the kudu client the client http://gerrit.cloudera.org:8080/#/c/17518/1//COMMIT_MSG@19 PS1, Line 19: It would be great to have an example of the error message before and after the patch. http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java: http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@373 PS1, Line 373: failure.getMessage() + operation.getTable().getName() Here and below: how does this look like in practice? I'm not sure appending the table name in the end even without a space make the error message more readable. What about using something like below for the message: String.format("%s: %s", operation.getTable().getName(), failure.getMessage()) ? http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@811 PS1, Line 811: RowError rowError = new RowError(status, operation); : if (e instanceof NonCoveredRangeException) { : rowError = new RowError(Status.NotFound(e.getMessage() + operation.getTable().getName()), operation); : } nit: why not to do that using the if/else clause? Otherwise, there might be duplicate 'new' calls. As an alternative, maybe build separate Status depending on the type of exception (i.e. use the if/else clause for Status), and then have just single 'new RowError(status, operation)'? http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeException.java File java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeException.java: http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeException.java@37 PS1, Line 37: ] nit: usually it's a non-inclusive bound for a partition, so maybe use ')' character instead of ']'? http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeException.java@37 PS1, Line 37: Accessed nit: accessed Why to capitalize? http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java: http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@559 PS1, Line 559: session.apply(insert); : session.flush(); nit: does it make sure those return null (or whatever) as expected in case of AUTO_FLUSH_BACKGROUND? http://gerrit.cloudera.org:8080/#/c/17518/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@568 PS1, Line 568: flush nit: 'flushResult' or 'result' might be a better name for this variable -- To view, visit http://gerrit.cloudera.org:8080/17518 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia24582de6b060e908f5ecbc46e2638b95cd567b3 Gerrit-Change-Number: 17518 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 26 May 2021 21:47:24 +0000 Gerrit-HasComments: Yes
