Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11815 )
Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala: http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@138 PS2, Line 138: kuduContext.timestampAccumulator.add(kuduContext.syncClient.getLastPropagatedTimestamp) > This is so that if you're using the same KuduContext for a backup job and a This was added because it brings KuduBackupRDD closer to KuduRDD. Given we are using READ_AT_SNAPSHOT mode and not RYW mode, I don't think this has any direct impact on backups even for testing. In the real world, each backup job will be a separate job, so this line has even less impact. http://gerrit.cloudera.org:8080/#/c/11815/2/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/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@356 PS2, Line 356: error > nit: why not use 'info' instead of 'error'? oh, good catch. This was just for my experimenting. I intended to lower back to info. http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@364 PS2, Line 364: plus a logical clock > This detail isn't relevant to the problem/solution discussion, is it? No, I just wanted to be correct. I can remove it. http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@364 PS2, Line 364: This means : // if the data is inserted with a fraction of a ms remaining it could be truncated and : // unread. > In practice this is always true; what are the odds that a timestamp value o Right. will update. http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@368 PS2, Line 368: nowMs + 1 > Does this mean the default value for timestampMs in KuduBackupOptions shoul I don't think so. In the real world this issue can't occur and defaulting to currentTimeMillis makes sense. In the real world writing data and submitting a spark job in < 1ms is impossible. -- To view, visit http://gerrit.cloudera.org:8080/11815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d Gerrit-Change-Number: 11815 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 06 Nov 2018 15:27:10 +0000 Gerrit-HasComments: Yes
