Hao Hao 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 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@197 PS9, Line 197: : // > this sentence is non-sensical. also max/latest is bound to be confusing. re Makes sense. Updated. http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@201 PS9, Line 201: // errors, timeouts, or leadership issues. : // 3) 'deadline' (if initialized) elapses. : // : // NOTE: 'rpc_timeout' is a per-call timeout, while > also non-sensical; also see my comment above Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-test.cc@992 PS9, Line 992: ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 27, kNoBound)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 15)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 10)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 20)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 14, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 30, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kTabletsNum * kRowsPerTablet, : kNoBound)); : } : INSTANTIATE_TEST_CASE_P(Params, ScanMultiTabletParamTest, : testing::ValuesIn(read_modes)); : : TEST_F(ClientTest, TestScanEmptyTable) { : K > can you make this a parameterized test that received the read mode as input Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1740 PS9, Line 1740: /// When @c READ_YOUR_WRITES is specified, the server will perform a server-local > I still think there is room to simplify these docs. I would prefer if they Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1743 PS9, Line 1743: minimize > nit: verbage (... ensures ... minimize sounds weird) Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1745 PS9, Line 1745: > What does this mean in terms of an application developer reading this doc? Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scan_token-internal.cc@253 PS9, Line 253: LOG(FATAL) << "Ignoring snapshot timestamp since not in " : "READ_AT_SNAPSH > should we validate this on the config? i.e. don't allow to pass a snapshot Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scanner-internal.cc@330 PS9, Line 330: if (configuration_.has_snapshot_timestamp()) { : LOG(FATAL) << "Ignoring snapshot timestamp since " : "not in READ_AT_SNAPSHOT mode."; : } > same comment I did elsewhere Done -- 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: 10 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alex Rodoni <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Wed, 21 Feb 2018 05:24:02 +0000 Gerrit-HasComments: Yes
