Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13246 )
Change subject: KUDU-2810: [backup] Add workaround DELETE IGNORE support ...................................................................... Patch Set 1: (4 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. 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? 1. Do a backup (an empty one) 2. Insert a row. 3. Delete the row. 4. Do an incremental backup 5. Try to restore the table. The row deleted in step 4 will generate a delete operation during restore but the row won't be present in the table as restored from the backup in step 2. 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? 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 would be nice. My understanding from reading the patch is that some flush modes flush individual operations and some flush batches, and each has some of its own separate handling of per-row errors. -- 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: 1 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 17:58:02 +0000 Gerrit-HasComments: Yes
