David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

Line 591:     ASSERT_OK(builder.SetSnapshotRaw(ts));
> Currently that's not the case -- the ScanConfiguration object underneath th
maybe I'm missing something: if the scan tokens are propagating timestamps 
correctly and we don't set a timestamp on the snapshot scan then the server 
should choose a timestamp that is higher than the last write. that is the point 
of timestamp propagation in scan tokens no?


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);
> What's exactly tricky?  I don't see any trickiness here -- please check my 
the trickyness stems from the fact that you could get timeouts that are 
unrelated with what you are testing and that the test is only doing an indirect 
check. My comment was: can you do direct check somewhere? Its cool if not 
possible but if it is it would be preferable


-- 
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 <aser...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-HasComments: Yes

Reply via email to