Alexey Serbin has posted comments on this change. Change subject: [i-tests] scan token timestamp propagation test ......................................................................
Patch Set 1: (8 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 It's not that crucial, actually. I just put it here to be in line with the rest of the list. If it makes people wonder what it is, I will remove this step since it's not that crucial for understanding the idea behind the test. 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 scenari Done PS1, Line 540: , acting as an external entity, > what does this mean? Well, nothing particular -- all clients act as external entities. The idea was to express that it's possible to pass scan tokens between unrelated client applications. I'll remove this wording. PS1, Line 541: which clock is advanced. > missing some words/rephrase? Done 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 I wanted to point that the test would not hang, but ran fast and reported an error right away due the the described facts. PS1, Line 555: TEST_F(ConsistencyITest, DISABLED_TestScanTokenTimestampPropagation) > this fails all the time without the fix, right? Yes, it does fail without the fix, 100% Line 591: ASSERT_OK(builder.SetSnapshotRaw(ts)); > Why do you have to set the raw snapshot timestamp? Shouldn't it be propaga Currently that's not the case -- the ScanConfiguration object underneath the KuduScanTokenBuilder is not implicitly affected while building the tokens. That sort of 'automation' is available only for KuduScanner while calling OpenTablet() method (i.e. while performing a scan operation at the table). However, you make a good point here -- may be we should make KuduScanTokenBuilder and KuduScanner similar in that regard. David, what do you think? 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 a What's exactly tricky? I don't see any trickiness here -- please check my previous comment. -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
