Andrew Wong 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 Done 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? We do in the AsyncKuduClient. I defined it again here since it seems a bit strange that we'd otherwise force users to do something like: (KuduClient.getLastPropagatedTimestamp() == AsyncKuduClient.NO_TIMESTAMP) Would having an alternative for statements like ^that warrant the redundancy? This is noted in the commit message. 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 timestam Fair point, I'd originally begun adding some tests elsewhere, but opted to move to a separate test. Will do. -- 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 <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
