Alexey Serbin has posted comments on this change. Change subject: [scan] test for reusing snapshot timestamp when not set ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5163/2//COMMIT_MSG Commit Message: PS2, Line 7: [scan] test for reusing snapshot timestamp when not set > maybe mention the jira. something like: KUDU-XXXX Integration test for time Done PS2, Line 12: The test would fail because the fix for KUDU-1189 is absent, that's why : it's disabled for a while. The fix is coming in one of the next changes, : which will enable the test. > How about: The test is disabled as it currently fails. A follow up patch wi Yep, that sounds better -- it's terse and explains the essence. http://gerrit.cloudera.org:8080/#/c/5163/2/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: PS2, Line 226: UpdateClockForTabletAtKey > How about: UpdateClockForTabletHostingKey ok, sure. I just tried to keep it shorter :) PS2, Line 297: > missing a word? "about"? Done PS2, Line 435: shared_ptr<KuduClient> client; : ASSERT_OK(cluster_->CreateClient(nullptr, &client)); : // Scan the table at a snapshot: let the servers pick the timestamp. : shared_ptr<KuduTable> table; : ASSERT_OK(client->OpenTable(table_name_, &table)); > nit add a CreateClientAndOpenTable helper? That would decrease the boilerplate code in this test a bit (one less line for those blocks), but I don't think it brings in any value from the semantic point of view. I.e., instead of shared_ptr<KuduClient> client; ASSERT_OK(cluster_->CreateClient(nullptr, &client)); shared_ptr<KuduTable> table; ASSERT_OK(client->OpenTable(table_name_, &table)); there would be shared_ptr<KuduClient> client; shared_ptr<KuduTable> table; ASSERT_OK(OpenTableWithNewClient(&client, &table)); That's said, I would leave this as is. However, if you feel strongly about that, I'll change it. PS2, Line 490: // Insert an additional row into the first tablet. : ASSERT_OK(InsertTestRows(client.get(), table.get(), 1, 1)); > what is the point of the additional write? This is to check that the timestamp is taken from the first tablet server: since now the clocks of both tablet servers are ahead of the timestamps of the inserted rows so far, there would be no way to tell which server's clock is used for the scan: in either case, there will be two result rows. Now, once we add a new row into the first tablet, given the big time margin provided by the current clock offset, we should see different outcomes from the subsequent scan: * if the timestamp is taken from the first server, there should be three rows in the result * if the timestamp is taken from the second server, there should be just two rows in the result. I'll add the corresponding comment into the code. PS2, Line 521: const uint64_t ts_scan = cfg.snapshot_timestamp(); : const uint64_t ts_lo = client->GetLatestObservedTimestamp(); : const MonoDelta t_diff = t_after_scan - t_before_scan; : // An additional check that relies on the assumption that : // the difference between servers' clocks is much bigger compared : // with the scan time. If the assumption does not hold, this check : // does not prove anything, but it does not fail neither. : ASSERT_GE(t_diff.ToMicroseconds(), ts_lo - ts_scan); > sorry I missed this before, but isn't this comparing a hybrid time differen ok, good point. Somehow I overlooked the fact the hybrid timestamp oranges are not those wall clock apples :) Thank you for pointing at that -- will fix. -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 2 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
