Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13246 )
Change subject: KUDU-2810: [backup] Add workaround DELETE IGNORE support ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/13246/2/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java File java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java: http://gerrit.cloudera.org:8080/#/c/13246/2/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java@70 PS2, Line 70: Set<ErrorCode> ignoredErrors = new HashSet<>(); ErrorCode is defined as an enum in protobuf, so can this be an EnumSet? http://gerrit.cloudera.org:8080/#/c/13246/2/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java File java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java: http://gerrit.cloudera.org:8080/#/c/13246/2/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java@159 PS2, Line 159: Nit: extra space here. Same in setIgnoreAllNotFoundRows(). http://gerrit.cloudera.org:8080/#/c/13246/2/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java@185 PS2, Line 185: void setIgnoreAllNotFoundRows(boolean ignoreAllNotFoundRows); In your opinion do these new APIs make it more difficult to implement KUDU-1563? http://gerrit.cloudera.org:8080/#/c/13246/2/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/13246/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@113 PS2, Line 113: public void testIgnoreAllNotFoundRows() throws Exception { This is a weird structure for this unit test: 1. Why bother testing multiple flush modes? There doesn't appear to be any intersection between the functionality being tested and the flush mode. It leads to weird test code like sleeping until AUTO_FLUSH_BACKGROUND is done. 2. Seems like a minimal test would just insert a row, then delete it three times: 2a. First delete succeeds. 2b. Second delete returns a row error. Then you change the session configuration. 2c. Third delete succeeds. Maybe repeat the whole sequence for two rows if you want to test that the configuration doesn't somehow only apply to only one row. -- To view, visit http://gerrit.cloudera.org:8080/13246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib718b72a73b06d9b9dc809fde3c52a1d498ceb23 Gerrit-Change-Number: 13246 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 06 May 2019 18:01:09 +0000 Gerrit-HasComments: Yes
