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: (4 comments) http://gerrit.cloudera.org:8080/#/c/8823/15/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: http://gerrit.cloudera.org:8080/#/c/8823/15/src/kudu/integration-tests/consistency-itest.cc@117 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. http://gerrit.cloudera.org:8080/#/c/8823/15/src/kudu/integration-tests/consistency-itest.cc@132 PS15, Line 132: for (int j = 0; j < 3; j++) { : CHECK_OK(GetRowCount(table.get(), KuduScanner::READ_YOUR_WRITES, : 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 months) http://gerrit.cloudera.org:8080/#/c/8823/15/src/kudu/integration-tests/consistency-itest.cc@776 PS15, Line 776: const KuduClient::ReplicaSelection replica_selectors[] = { : KuduClient::LEADER_ONLY, : KuduClient::CLOSEST_REPLICA, : KuduClient::FIRST_REPLICA, : }; nit maybe move this to L745? http://gerrit.cloudera.org:8080/#/c/8823/15/src/kudu/integration-tests/consistency-itest.cc@790 PS15, Line 790: // Verify that when multiple clients running simultaneously, a single : // could achieve read-your-writes/read-your-reads on READ_YOUR_WRITES : // 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
