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 12: (23 comments) http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG@15 PS12, Line 15: choose the newest timestamp : within the staleness bound that allows execution of the reads without : being blocked by the in-flight transactions > might be worth adding "if possible" Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@236 PS12, Line 236: subject to the propagated timestamp bound > This wording is pretty hard to a casual reader (or even me) to understand. Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@237 PS12, Line 237: newest timestamp within > latest timestamp higher than Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@242 PS12, Line 242: the timestamp > a timestamp Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@244 PS12, Line 244: should use it > Is this something that end-users have to worry about, or can we word this s Updated. Currently, we use current clock time of the server as the propagation time. This would bump up the propagation time unnecessarily for the next read in this mode. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@245 PS12, Line 245: wait > nit: waiting Done http://gerrit.cloudera.org:8080/#/c/8804/11/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/11/src/kudu/tserver/tablet_server-test.cc@185 PS11, Line 185: void ScanYourWritesTest(uint64_t propagated_timestamp, ScanResponsePB* resp); > warning: parameter 'propagated_timestamp' is const-qualified in the functio Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: PS12: > We are missing a C++ client test in this patch. Is that excluded on purpose C++ client test in the following CR https://gerrit.cloudera.org/#/c/8823/ http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TEST_F(TabletServerTest, TestScanYourWrites) { > nit: add a comment at the top of this test describing what the test does Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TestScanYourWrites > It worries me a bit that most tests of this functionality overwhelmingly te Added more integration tests in C++ client which would definitely fail with READ_LATEST. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TestScanYourWrites > I agree that a deterministic test would be nice and I think it should be po Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1927 PS12, Line 1927: e > nit: missing period Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1958 PS12, Line 1958: TEST_F(TabletServerTest, TestScanYourWrites_WithoutPropagatedTimestamp) { > nit: add test comment Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1960 PS12, Line 1960: perform a write > nit: use capitalization and punctuation for code comments per https://googl Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1972 PS12, Line 1972: perform a write > punctuation Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@155 PS12, Line 155: // it exceeds the maximum allowed clock synchronization error time. > Can we add a note about the context, i.e. why this matters? Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@158 PS12, Line 158: if > s/if/that/ Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2113 PS12, Line 2113: > add: DCHECK(s.ok()) ? I think even for status that returns 'NotSupported' can reach here. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114 PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed : // to be higher than 'tmp_snap_timestamp'. > I'm having trouble following this: Good question. TBH, this is refactored from previous code, so I am not sure about the answers, but I will try my best. My understanding of "not obtained" means the current clock type (logical clock) does not support this function. It returns 'NotSupported' status and FLAGS_scanner_allow_snapshot_scans_with_logical_timestamps is enabled. For the second question, maybe David can shed some light? http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2133 PS12, Line 2133: that > than Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2135 PS12, Line 2135: much > far Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2164 PS12, Line 2164: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp)); > Hmm, we already updated the clock on line 2137 so AFAICT the only way this Right, that is the purpose. I think they are two different things. Updating the clock in L2137 is to ensure the server's clock is not lagging behind the others, while here is validating the chosen timestamp is not too far in the future. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2170 PS12, Line 2170: VerifyNotAncientHistory > same here; is there some way to check this before updating the clock, or we Same answer as above, it is only validating the chosen timestamp. -- 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: 12 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 15 Feb 2018 01:21:36 +0000 Gerrit-HasComments: Yes
