Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode ...................................................................... Patch Set 2: (11 comments) mostly nits http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@222 PS2, Line 222: pick an arbitrarily nit: "arbitrarily pick" or "pick an arbitrary" BTW, what's arbitrary about the snapshot? It seems like it's not arbitrary since it's subject to a specific bound. Or is it selected randomly within that bound? Based on the rest of the comment, it seems like the chosen snapshot is pretty specific: "the newest timestamp within the staleness bound" http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@225 PS2, Line 225: staleness bound that allows execution of the reads This "staleness bound" isn't mentioned anywhere else, so I'm not sure exactly what it's referring to. Mind rewording this a bit? Same in other places where "staleness bound" is used. Maybe: "the newest timestamp that would allow reads at the provided propagated timestamp..." or something? http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@229 PS2, Line 229: Boundedly stale reads nit: maybe "Reads in this mode"? http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@a1616 PS2, Line 1616: : : nit: various spurious deletions of newlines http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1835 PS2, Line 1835: TestBoundedScan nit: this should be renamed (same with other tests) http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1866 PS2, Line 1866: the no snapshot nit: typo, same in other tests. http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1869 PS2, Line 1869: resp.has_propagated_timestamp() nit: maybe pull into a local variable? http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1878 PS2, Line 1878: num_rows micro-nit: kNumRows? Or maybe use one row instead of 100? That way we only need to check one result, unless checking multiple is important. http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1899 PS2, Line 1899: req.set_batch_size_bytes(0); // so it won't return data right away Is this needed here? I thought we only needed it to postpone draining the scanner or something (not sure). http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1968 PS2, Line 1968: ASSERT_NO_FATAL_FAILURE nit: I think you can use the shorter "NO_FATALS" here http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2096 PS2, Line 2096: BOUNDED_READ nit: READ_OWN_WRITES -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 14 Dec 2017 03:35:42 +0000 Gerrit-HasComments: Yes
