Kevin McCarthy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16276 )
Change subject: [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property Added property snapshotTimestampMs to spark read options which will allow consistant scans when timestamp is set before the first dataFrame read. ...................................................................... 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. Done http://gerrit.cloudera.org:8080/#/c/16276/1//COMMIT_MSG@8 PS1, Line 8: > Can you add a short paragraph describing the change. Done 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 That was my initial thought. I will us MS and multiply by 1000 before passing to snapshotTimestampMicros 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? Currently the only way to set READ_AT_SNAPSHOT as the readMode is by using CLOSEST_REPLICA for scanLocality. Would you recommend exposing readMode or adding to the comments that this property will only be set when scanLocality is set to CLOSEST_REPLICA? 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 Done 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? I'm not sure what my original intent was. I will move back since it isn't related to this change 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 i Done 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 It appears that this is an encoded value. Is there a good way to decode into a format that can be passed into snapshotTimestampMicros? "@return a long indicating the specially-encoded last timestamp received from a server" -- 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: Kevin McCarthy <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 03 Aug 2020 18:06:44 +0000 Gerrit-HasComments: Yes
