David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8823 )

Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode

Patch Set 15:


File src/kudu/integration-tests/consistency-itest.cc:

PS15, Line 117:   void ScannerThread(KuduClient::ReplicaSelection selection,
              :                      int rows_to_insert,
              :                      int first_row) {
how about we make this a fast/slow test so that we get a bit more coverage in 
jenkins but keep the local test fast? that is another argument indicating how 
many scans to perform, e.g. 'scans_to_perform' then insert one single row per 
orund outer loop and perform 'scans_to_perform' in the inner loop.

the call site would maybe pass 3/3 in the fast tests and 10/10 on the slow 
tests (maybe even 100/100 if it's not too slown in TSAN).

The point is that larger strings of tests are more likely to show anomalies.

PS15, Line 132: for (int j = 0; j < 3; j++) {
              :         CHECK_OK(GetRowCount(table.get(), 
              :                              selection, 0, &row_count));
              :         EXPECT_LE(expected_count, row_count);
              :         expected_count = row_count;
              :       }
maybe add an explanation of why we're doing this and how this verified RYR, 
otherwise might not be clean to the next reader (or to ourselves in a few 

PS15, Line 776: const KuduClient::ReplicaSelection replica_selectors[] = {
              :     KuduClient::LEADER_ONLY,
              :     KuduClient::CLOSEST_REPLICA,
              :     KuduClient::FIRST_REPLICA,
              : };
nit maybe move this to L745?

PS15, Line 790: // Verify that when multiple clients running simultaneously, a 
              : // could achieve read-your-writes/read-your-reads on 
              : // scan mode no matter which replica is selected.
maybe explain summarily the general strategy of the test?
these tests that are testing subtle things become hard to reason about a while 
after they're written and comments/docs help.

for guidance, see all of the other tests in this file.

To view, visit http://gerrit.cloudera.org:8080/8823
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1
Gerrit-Change-Number: 8823
Gerrit-PatchSet: 15
Gerrit-Owner: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Alex Rodoni <arod...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Comment-Date: Sat, 24 Feb 2018 02:03:12 +0000
Gerrit-HasComments: Yes

Reply via email to