Alexey Serbin 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: (9 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 ? 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@1843 PS2, Line 1843: RpcController rpc; nit: move this under the 'Send the call' scope. http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1855 PS2, Line 1855: s nit: make it capital 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 that? http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1922 PS2, Line 1922: post ? 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. 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? http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2153 PS2, Line 2153: BOUNDED_READ nit: READ_OWN_WRITES ? 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 temporary and use that for the comparison logic? -- 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: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 13 Dec 2017 17:19:32 +0000 Gerrit-HasComments: Yes
