Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode ...................................................................... Patch Set 16: (10 comments) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726 PS16, Line 1726: > What happened to this test? Maybe I'm missing something, but it seems this Since ScannerOpenWhenServerShutsDownParamTest should also cover the scenario of this test case. I removed it as it is duplicated. http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157 PS16, Line 157: TimeStamp > nit: in the code around, we have Timestamp as a type; maybe, name it consis Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161 PS16, Line 161: PickAndVerifyTimeStamp > nit: ditto, PickAndVerifyTimestamp(...) ? Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162 PS16, Line 162: tablet::TabletReplica* > Could it be const tablet::TabletReplica& ? Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757 PS16, Line 1757: case READ_YOUR_WRITES: > nit: per coding standard, add // fallthrough Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042 PS16, Line 2042: case READ_AT_SNAPSHOT: > nit: per coding standard, add // fallthrough Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117 PS16, Line 2117: . > s/./,/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: initiated > s/initiated with/default-constructed to/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: Since > s/Since/since/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > is that guaranteed by something? As we update the clock based on propagated timestamp at L2140, if (propagation ts + 1) is a timestamp too far in the future, which essentially means (propagation TS + 1 > propagation TS + FLAGS_max_clock_sync_error_usec). -- 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: 16 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[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-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 23:50:48 +0000 Gerrit-HasComments: Yes
