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 9: (6 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: from servers that only for all : // opened scans with READ_YOUR_WRITES mode. this sentence is non-sensical. also max/latest is bound to be confusing. remind me again why we need the two? I thought we'd do something like: - At the beginning of a scan, get the last propagated timestamp (say t0) and pass it to the scan config (we'll always use this one and won't read the last propagated timestamp from the client for the duration of the scan). - With each scan request, for each tablet, send t0. Each server is free to choose a timestamp that is bigger than t0 exactly as implemented in your server side patch. - Each scan response includes the chosen timestamp on the server, ts. - Each scan response updates the "last propagated timestamp" with ts, making sure we have RYR. http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@201 PS9, Line 201: // Updates the max observed timestamp from servers that only for all : // opened scan with READ_YOUR_WRITES mode, if the given timestamp is : // greater than the one stored. : void UpdateMaxObservedTimestamp(uint64_t timestamp); also non-sensical; also see my comment above 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: // Check counts changed accordingly using both READ_LATEST and : // READ_YOUR_WRITES scan mode. : for (auto read_mode : kReadModes) { : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kNoBound, kNoBound)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kNoBound, 15)); : 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)); : } can you make this a parameterized test that received the read mode as input? 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@1743 PS9, Line 1743: minimize nit: verbage (... ensures ... minimize sounds weird) 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(WARNING) << "Ignoring snapshot timestamp since not in " : "READ_AT_SNAPSHOT mode."; should we validate this on the config? i.e. don't allow to pass a snapshot timestamp (or to have on set) when choosing RYW? we could return an error status there and then LOG(FATAL) here 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(WARNING) << "Ignoring snapshot timestamp since " : "not in READ_AT_SNAPSHOT mode."; : } same comment I did elsewhere -- 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: 9 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-Comment-Date: Tue, 20 Feb 2018 20:26:12 +0000 Gerrit-HasComments: Yes
