Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16276 )
Change subject: Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/16276/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16276/1//COMMIT_MSG@7 PS1, Line 7: Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property Add `KUDU-3177:` prefix. http://gerrit.cloudera.org:8080/#/c/16276/1//COMMIT_MSG@8 PS1, Line 8: Can you add a short paragraph describing the change. http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala: http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala@74 PS1, Line 74: val SNAPSHOT_TIMESTAMP_MICROS = "kudu.snapshotTimestampMicros" Would it make sense to use millis given that is a more common unit for time and matches the backup job behavior? http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala: http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala@71 PS1, Line 71: if (options.scanLocality == ReplicaSelection.CLOSEST_REPLICA) { What about the case where scanLocality is LEADER_ONLY? http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala: http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala@45 PS1, Line 45: * to allow repeatable reads. If not set, the timestamp is generated by the server. nit: line too long http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala@54 PS1, Line 54: useDriverMetadata: Boolean = defaultUseDriverMetadata, Why did this move? http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala: http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@771 PS1, Line 771: def testReadDataFrameAtSnapshot() { Can you add a test that shows what happens when a snapshot is passed that is too old or too far in the future? http://gerrit.cloudera.org:8080/#/c/16276/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@773 PS1, Line 773: val timestamp = System.currentTimeMillis() * 1000 I think you should use `client.getLastPropagatedTimestamp()` to get a real snapshot time for this test similar to the diff scan tests linked below: https://github.com/apache/kudu/blob/master/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java#L492 -- To view, visit http://gerrit.cloudera.org:8080/16276 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0 Gerrit-Change-Number: 16276 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin McCarthy <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 03 Aug 2020 15:05:41 +0000 Gerrit-HasComments: Yes
