David Ribeiro Alves 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 timestamp reuse on snapshot scans with no timestamp set. 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 will fix the bug and enable the test. 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 PS2, Line 297: missing a word? "about"? 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? 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? 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 difference with a physical time difference? That doesn't sound right and tbh I don't think this check is adding much to the test. Same goes for the previous block. -- 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: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
