Abhishek Chennaka 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: (9 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 Done 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: Done 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 Done 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 appendin So the failure.getMessage() actually has a space at the end of the string. So the end result will be something like: "Accessed Range Partition ([0x80000064, 0x800000C8]) does not exist in table TestKuduSession" But I guess thats not the most elegant approach. Will use the one provided by you 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 b Done 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: Accessed > nit: accessed It might appear like it is at the beginning of the sentence, maybe? Wrote a patch to change it to "accessed range partition" 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 ')' c Yes, thanks for pointing that out. Correcting it in the next patch 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 Will do through assertTrue() for the apply method. Flush returns List<OperationResponse> though like below. 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 Done -- 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: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 27 May 2021 21:39:43 +0000 Gerrit-HasComments: Yes
