You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Kevin McCarthy (Code Review)" <ge...@cloudera.org> on 2020/08/03 14:27:11 UTC
[kudu-CR] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Kevin McCarthy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16276
Change subject: Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
......................................................................
Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
4 files changed, 47 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/16276/1
--
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: newchange
Gerrit-Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
Gerrit-Change-Number: 16276
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
[kudu-CR] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 03 Aug 2020 15:05:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [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.
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16276/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16276/2//COMMIT_MSG@8
PS2, Line 8:
nit: add a blank line between subject and description.
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) {
> Currently the only way to set READ_AT_SNAPSHOT as the readMode is by using
I think you could set READ_AT_SNAPSHOT if `options.scanLocality == ReplicaSelection.CLOSEST_REPLICA || options.snapshotTimestampMicros.isDefined` and then move setting snapshotTimestampMicros outside of this if statement.
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@773
PS1, Line 773: val timestamp = System.currentTimeMillis() * 1000
> It appears that this is an encoded value. Is there a good way to decode int
You can use the HybridTimeUtil class to convert as needed:
https://github.com/apache/kudu/blob/master/java/kudu-client/src/main/java/org/apache/kudu/util/HybridTimeUtil.java
You can see it used in the BackupRDD here:
https://github.com/apache/kudu/blob/master/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala#L71-L77
--
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 <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 03 Aug 2020 19:43:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [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.
Posted by "Kevin McCarthy (Code Review)" <ge...@cloudera.org>.
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 <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 03 Aug 2020 18:06:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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
......................................................................
Patch Set 3:
(4 comments)
Thank you for addressing all the feedback. Just a few small nits, otherwise it looks good to me.
http://gerrit.cloudera.org:8080/#/c/16276/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16276/3//COMMIT_MSG@9
PS3, Line 9: Added property snapshotTimestampMs to spark read options which will allow consistant scans
nit: wrap around 70-ish characters
http://gerrit.cloudera.org:8080/#/c/16276/3/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/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@767
PS3, Line 767: assert
Use assertTrue here and below.
http://gerrit.cloudera.org:8080/#/c/16276/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@804
PS3, Line 804: Thread.sleep(1000)
Might be worth sleeping an extra second to avoid flakiness in the test.
http://gerrit.cloudera.org:8080/#/c/16276/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala:
http://gerrit.cloudera.org:8080/#/c/16276/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala@35
PS3, Line 35: import org.apache.kudu.util.{CharUtil, DateUtil, DecimalUtil, HybridTimeUtil}
nit: leave as separate import lines
--
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: 3
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 Aug 2020 16:04:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Kevin McCarthy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16276
to look at the new patch set (#4).
Change subject: [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
......................................................................
[KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions
as optional property
Added property snapshotTimestampMs to spark read options which will
allow consistant scanswhen timestamp is set before the first
dataFrame read.
Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
5 files changed, 100 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/16276/4
--
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: newpatchset
Gerrit-Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
Gerrit-Change-Number: 16276
Gerrit-PatchSet: 4
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [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.
Posted by "Kevin McCarthy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16276
to look at the new patch set (#2).
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.
......................................................................
[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.
Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
4 files changed, 91 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/16276/2
--
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: newpatchset
Gerrit-Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
Gerrit-Change-Number: 16276
Gerrit-PatchSet: 2
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Kevin McCarthy (Code Review)" <ge...@cloudera.org>.
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
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16276/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16276/3//COMMIT_MSG@9
PS3, Line 9: Added property snapshotTimestampMs to spark read options which will allow consistant scans
> nit: wrap around 70-ish characters
Done
http://gerrit.cloudera.org:8080/#/c/16276/3/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/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@767
PS3, Line 767: assert
> Use assertTrue here and below.
Done
http://gerrit.cloudera.org:8080/#/c/16276/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@804
PS3, Line 804: Thread.sleep(1000)
> Might be worth sleeping an extra second to avoid flakiness in the test.
Done
http://gerrit.cloudera.org:8080/#/c/16276/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala:
http://gerrit.cloudera.org:8080/#/c/16276/3/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala@35
PS3, Line 35: import org.apache.kudu.util.{CharUtil, DateUtil, DecimalUtil, HybridTimeUtil}
> nit: leave as separate import lines
Done
--
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: 3
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 Aug 2020 16:51:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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
......................................................................
Patch Set 4: Code-Review+2
--
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: 4
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 Aug 2020 17:59:24 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Kevin McCarthy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16276
to look at the new patch set (#3).
Change subject: [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
......................................................................
[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.
Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
5 files changed, 100 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/16276/3
--
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: newpatchset
Gerrit-Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
Gerrit-Change-Number: 16276
Gerrit-PatchSet: 3
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Kevin McCarthy (Code Review)" <ge...@cloudera.org>.
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
......................................................................
Patch Set 1:
(2 comments)
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) {
> I think you could set READ_AT_SNAPSHOT if `options.scanLocality == ReplicaS
Done
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@773
PS1, Line 773: val timestamp = System.currentTimeMillis() * 1000
> You can use the HybridTimeUtil class to convert as needed:
Done
--
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 <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 03 Aug 2020 21:47:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16276 )
Change subject: [KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions as optional property
......................................................................
[KUDU-3177] Added kudu.snapshotTimestampMicros to kudu spark readOptions
as optional property
Added property snapshotTimestampMs to spark read options which will
allow consistant scanswhen timestamp is set before the first
dataFrame read.
Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
Reviewed-on: http://gerrit.cloudera.org:8080/16276
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala
5 files changed, 100 insertions(+), 3 deletions(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I00862c0e174a964efc6cab0b8141b1ac5a1bebc0
Gerrit-Change-Number: 16276
Gerrit-PatchSet: 5
Gerrit-Owner: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kevin McCarthy <00...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)