Grant Henke 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/13246/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala: http://gerrit.cloudera.org:8080/#/c/13246/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@101 PS1, Line 101: > nit: Extra space. This is on purpose, it allows IDEs to detect a multiline todo. http://gerrit.cloudera.org:8080/#/c/13246/1/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala: http://gerrit.cloudera.org:8080/#/c/13246/1/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@421 PS1, Line 421: // When restoring the table, delete half the rows after each job completes. : // This will force delete rows to cause NotFound errors and allow validation : // that they are correctly handled. > Can't we reproduce this without doing something fancy? I don't want this test to depend on the "bad" diff scan behavior because Adar is working on fixing that. Even after diff scan doesn't included incorrect deletes we need to ensure delete ignore works for retries. 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? Done http://gerrit.cloudera.org:8080/#/c/13246/1/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/1/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java@182 PS1, Line 182: <p> > What's the purpose of this tag? I copied it from above. It appears a couple times in the file. It just formats the javadoc html presentation slightly. 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(). This is on purpose, it allows IDEs to detect a multiline todo. 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- I don't think so. Technically both, ignoring errors and preventing errors, can exist and work together regardless of how we implement them. If IgnoreAllDuplicateRows functionality didn't already I would lean towards not adding this. Primarily because the performance of relying on filtering per-row errors is likely to be subpar. But because it does already exist, adding another instance of ignoring error codes doesn't add any more complexity or problems. http://gerrit.cloudera.org:8080/#/c/13246/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/13246/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@118 PS1, Line 118: for (SessionConfiguration.FlushMode mode : SessionConfiguration.FlushMode.values()) { > A one-sentence comment on why it's necessary to test all the flush modes wo Done 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: This is primarily a "copy" of the test logic for testIgnoreAllDuplicateRows. Per Wills review I added a comment explaining the motivation for testing each flush type. -- 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:28:03 +0000 Gerrit-HasComments: Yes
