Alexey Serbin has posted comments on this change. Change subject: KUDU-1189 if not set, use timestamp from first server ......................................................................
Patch Set 3: (33 comments) http://gerrit.cloudera.org:8080/#/c/5143/3//COMMIT_MSG Commit Message: PS3, Line 7: if not set, use timestamp from first server > Maybe rephrase to: Snapshot scans: reuse snapshot timestamp when not set Done PS3, Line 9: KUDU-1189 On reads at a snapshot that touch multiple tablets, without : the user setting a timestamp, use the timestamp from the first server : for following scans > weird wrapping, I guess you were going for an "extended" title. maybe not n As I understand, that's the title for KUDU-1189 as is. I'll remove the extra spaces for the next couple of lines to update the wrapping. PS3, Line 15: > nit: extra space Done PS3, Line 15: read > s/read/scan Done PS3, Line 19: more consistency > what is "more consistency"? :) 'more consistency' is close to nonsense and too fancy, I agree :) The Java client part hasn't been updated. I'm thinking to do that in a separate changelist. http://gerrit.cloudera.org:8080/#/c/5143/3/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: PS3, Line 75: TimestampPropagationTest > I missed this in the previous patch, but can you rename this test to Consis Done PS3, Line 164: GetTabletIdForKeyValue > not from this patch, but maybe rephrase to GetTabletIdsForKeyIntervals? That sound like a good idea. I'll rename it here and remove the 'table_name' parameter, while the generalization for key intervals will be in a separate changelist. PS3, Line 273: TwoBatchesAndReadAtSnapshot > missed this on the previous patch. after the main test class rename can you Done PS3, Line 352: KUDU-1189 > to close this we also need to do it on the java client. is that already don That's not done yet; yes, in the TODO list. Thank you for the reminder. PS3, Line 354: In the context of the same scan : // operation, that timestamp is then used as the snapshot timestamp for the rest : // of the operations to read data from appropriate tablet servers. > nit: maybe rephrase this to: If the scan spans multiple tablets, the timest Done PS3, Line 358: The idea of the test is simple: have a scan spanned across two tablets : // where the clocks of the corresponding tablet servers are skewed. > I think we could do a better job explaining what exactly this test is doing Done PS3, Line 360: ServerAssignedScanTimestamp > does this test fail 100% without the rest of the patch? That I haven't verified yet. Will do and report on my findings. Good idea, BTW. PS3, Line 360: ServerAssignedScanTimestamp > rename to: TestSnapshotScanTimestampReuse Done PS3, Line 360: TEST_F(TimestampPropagationTest, ServerAssignedScanTimestamp) { > FWYW I think the approach we did for the previous patch where we're pushing Ah, I see. That's a great observation. Let me split this patch into two patches to have the same meaningful sequence. PS3, Line 361: maximum delta > you mean maximum error? also not really maximum since your dividing by 2 ri This is outdated comment, will need to update this one. The idea add the second scenario came later, but I didn't update this comment. PS3, Line 363: delta > s/delta/offset ? or something Done PS3, Line 384: dynamic_cast<HybridClock*>(peer->clock()) > nit: is cases such as this you could use: HybridClock* clock = DCHECK_NOTNU Done PS3, Line 398: were > s/were/was Done Line 401: // Now, perform the scan at READ_AT_SNAPSHOT where timestamp is not specified: > where _a_ timestamp Done PS3, Line 409: to > remove "to" Done PS3, Line 417: size_t > still not totally sure why you're using 'size_t' here. I thought size_t was a good type for row count since the count cannot be negative, and there might be huge numbers there (each batch can be small, but in total there might be > 2^32 rows total). If you think int is better anyway, I'll replace it with int. Let me do that in a separate changelist, though. PS3, Line 422: , > At this point (choose one: we/the test has) inserted... Done PS3, Line 440: ASSERT_LE(0, t_diff.ToMicroseconds()); > what is this trying to "sanity check"? also the comparison is slightly weir After some consideration, I decided to remove this check. Basically, I think it's not worth checking that the basic clock works as expected -- there should be a specific unit test for that. Here we operate on the assumption that the clock works as it should. PS3, Line 452: Swap the servers in the time scale: > remove Done PS3, Line 453: deltas > replace deltas with something else Done PS3, Line 464: parition > typo Done PS3, Line 465: dynamic_cast<HybridClock*>(peer->clock()); > use DCHECK_NOTNULL Done PS3, Line 467: two_deltas > why "two_deltas"? Done PS3, Line 469: UpdateClock > Change/Add AdvanceClockInTablet(string tablet_id, MonoDelta delta). Done PS3, Line 479: snapshot scan > snapshot scan's timestamp Done PS3, Line 498: ince the snapshot timestamp : // is taken from the second server's clock > why is it taken from the second server? isn't it that you now have advanced Woops, that's my bad -- it seems I've messed up too much with this comment. Sure, the timestamp is taken from the first server and its clock is now ahead of other server's clock, so that's the reason why we are about to see all the rows. Will fix. PS3, Line 507: ASSERT_LE(0, t_diff.ToMicroseconds()); > same weird comparison, use ASSERT_GE Done PS3, Line 508: Just another > remove "Just another" here and elsewhere. Done -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
