David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1826 add propagated timestamp to sync client ......................................................................
Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/6798/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: PS7, Line 237: @return a long indicating the last timestamp received from a server Can you mention that the timestamp can't be interpreted as absolute time (i.e that it's encoded in some special way). please also add that to the other client to keep the comments consistent http://gerrit.cloudera.org:8080/#/c/6798/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java: PS7, Line 44: NO_TIMESTAMP don't we have this constant defined somewhere else? http://gerrit.cloudera.org:8080/#/c/6798/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: PS7, Line 92: long initial_ts = syncClient.getLastPropagatedTimestamp(); : : // Check that the initial timestamp is consistent with the asynchronous client. : assertEquals(initial_ts, client.getLastPropagatedTimestamp()); : assertEquals(initial_ts, syncClient.getLastPropagatedTimestamp()); : : // Attempt to change the timestamp to a lower value. This should not change : // the internal timestamp, as it must be monotonically increasing. : syncClient.updateLastPropagatedTimestamp(initial_ts - 1); : assertEquals(initial_ts, client.getLastPropagatedTimestamp()); : assertEquals(initial_ts, syncClient.getLastPropagatedTimestamp()); : : // Use the synchronous client to update the last propagated timestamp and : // check with both clients that the timestamp was updated. : syncClient.updateLastPropagatedTimestamp(initial_ts + 1); : assertEquals(initial_ts + 1, client.getLastPropagatedTimestamp()); : assertEquals(initial_ts + 1, syncClient.getLastPropagatedTimestamp()); can you actually perform some writes or something to test that the timestamps get updated by the client when messages are received from the server. May adding these tests to the already existing timestamp tests would be easier. -- To view, visit http://gerrit.cloudera.org:8080/6798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibbfeb9526423f5acd0ef6482869d36cdd146e921 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
