Hao Hao 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: (20 comments) 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: BOUNDED_READ > READ_OWN_WRITES ? Done 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" Right, the picked snapshot is subject to the criteria you mentioned. Maybe 'arbitrary' is not a good word here. I will remove it. The intention was to specify the snapshot is not something specifically controlled by user, even the rule to chose one is specific. 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 exact Done 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"? Done 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 I thought only one line is needed between tests? 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) Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1843 PS2, Line 1843: RpcController rpc; > nit: move this under the 'Send the call' scope. Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1855 PS2, Line 1855: s > nit: make it capital Done 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. Done 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? I assume you mean 'resp.propagated_timestamp()'? http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1878 PS2, Line 1878: num_rows > micro-nit: kNumRows? Done 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 s Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1917 PS2, Line 1917: ASSERT_LT(pre_scan_ts.ToUint64(), resp.propagated_timestamp()); > Should the result scan contain any rows? Does it make sense to assert on t I do not think it makes sense to assert on that, since mvcc clean timestamp is hard to predefined. http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1922 PS2, Line 1922: post > ? Done 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 Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.h@156 PS2, Line 156: Status PickAndVerifyTimeStamp(const NewScanRequestPB& scan_pb, > nit: add comments on what these two methods do. Done 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@1619 PS2, Line 1619: BOUNDED_READ > nit: READ_OWN_WRITES? Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2096 PS2, Line 2096: BOUNDED_READ > nit: READ_OWN_WRITES Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2153 PS2, Line 2153: BOUNDED_READ > nit: READ_OWN_WRITES ? Done http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2162 PS2, Line 2162: tmp_snap_timestamp = tmp_snap_timestamp > mvcc_manager->GetCleanTimestamp() ? : tmp_snap_timestamp : mvcc_manager->GetCleanTimestamp(); > nit: maybe, call GetCleanTimestamp() just once, storing the result in the t Done -- 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 07:33:46 +0000 Gerrit-HasComments: Yes
