David Ribeiro Alves has posted comments on this change. Change subject: [i-tests] scan token timestamp propagation test ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: PS1, Line 397: * Discard the client object. why is it relevant to discard the client at the end? are you doing anything more after this? if so maybe say what it is? PS1, Line 526: // The real-world use-case behind this test is as following: : // : // * An external entity such as Spark job writes some data into the Kudu : // database. Then, it builds a scan token which would allow to fetch : // some related data at the specified timestamp. The scan token is : // serialized and output as a string. : // : // * Another external entity (e.g., another client application based : // on the Kudu C++ client API) is fed with the serialized scan token : // obtained at the previous step. It performs a scan operation built : // from the scan token it's fed with. maybe be a bit more abstract? i mean no need to be talking specific scenarios in the test IMO. just mention that we can get scan tokens from a client (c++/java) and use them on another client (c++/java) and that the lpt should be set properly. PS1, Line 540: , acting as an external entity, what does this mean? PS1, Line 541: which clock is advanced. missing some words/rephrase? PS1, Line 552: since the timeout for the scan operation : // is set to much shorter time interval aren't you using a mock clock that doesn't advance by itself? why does the timeout matter? PS1, Line 555: TEST_F(ConsistencyITest, DISABLED_TestScanTokenTimestampPropagation) this fails all the time without the fix, right? PS1, Line 606: shared_ptr<KuduClient> client; : ASSERT_OK(cluster_->CreateClient(nullptr, &client)); : shared_ptr<KuduTable> table; : ASSERT_OK(client->OpenTable(table_name_, &table)); : KuduScanner* scanner_raw; : ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token, : &scanner_raw)); : unique_ptr<KuduScanner> scanner(scanner_raw); : scanner->SetTimeoutMillis(scan_timeout_msec); : ASSERT_OK(scanner->Open()); : size_t row_num = 0; : while (scanner->HasMoreRows()) { : KuduScanBatch batch; : ASSERT_OK(scanner->NextBatch(&batch)); : row_num += batch.NumRows(); : } : EXPECT_EQ(0, row_num); I think basing the test on a timeout is tricky, is there a way to make an assertion on actual clock setting/timestamp movement? -- To view, visit http://gerrit.cloudera.org:8080/5219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
