Mike Percy 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:


File src/kudu/common/common.proto:

PS12, Line 236: subject to the propagated timestamp bound
This wording is pretty hard to a casual reader (or even me) to understand.

I think it would be more clear to say something like: Each tablet server will 
choose a timestamp to use for a server-local snapshot scan subject to the 
following criteria: (1) It will be higher than the propagated timestamp, (2) It 
will attempt to minimize latency caused by waiting for outstanding write 
transactions to complete.

PS12, Line 237: newest timestamp within
latest timestamp higher than

PS12, Line 242: the timestamp
a timestamp

PS12, Line 244: should use it
Is this something that end-users have to worry about, or can we word this so 
that it sounds like Kudu will take care of it? If the latter, how about

"The Kudu client library will use it as the propagated timestamp for subsequent 
reads" or something along those lines? However I'm not sure why we are making 
the distinction here between a normally propagated timestamp and this different 

PS12, Line 245: wait
nit: waiting

File src/kudu/tserver/tablet_server-test.cc:

We are missing a C++ client test in this patch. Is that excluded on purpose?

PS12, Line 1924: TestScanYourWrites
> It worries me a bit that most tests of this functionality overwhelmingly te
I agree that a deterministic test would be nice and I think it should be 
possible using two clients.

It would also be nice to have a non-deterministic test where you have two 
clients in RYW mode reading and scanning. Loop them concurrently in separate 
threads at least a certain number of rounds or amount of time, as well as until 
both of the following conditions hold, while also triggering leader elections 
to make it easier to hit anomalies:

1. neither has ever violated RYW locally
2. we have observed anomalies where if we had been using a strict serializable 
approach then certain writes would have appeared, but they don't because of the 
hidden channel (the other client).

PS12, Line 1924: TEST_F(TabletServerTest, TestScanYourWrites) {
nit: add a comment at the top of this test describing what the test does

PS12, Line 1927: e
nit: missing period

PS12, Line 1958: TEST_F(TabletServerTest, 
TestScanYourWrites_WithoutPropagatedTimestamp) {
nit: add test comment

PS12, Line 1960: perform a write
nit: use capitalization and punctuation for code comments per 

PS12, Line 1972: perform a write

File src/kudu/tserver/tablet_service.h:

PS12, Line 155:   // it exceeds the maximum allowed clock synchronization error 
Can we add a note about the context, i.e. why this matters?

nit: Also perhaps rename to ValidateSnapshotTimestamp() ? Usually Stamp in 
Timestamp is not capitalized in the Kudu code, and this seems specific to 
snapshot timestamps.

PS12, Line 158: if

File src/kudu/tserver/tablet_service.cc:

PS12, Line 2113:
add: DCHECK(s.ok()) ?

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:

1. What does "not obtained" mean? Is it like GetGlobalLatest() returned 
Status::Corrruption or something like that and so then we assume max_allowed_ts 
is still default-constructed?
2. Why is it guaranteed to by higher than 'tmp_snap_timestamp'? Can we explain 
what guarantees that and why?

PS12, Line 2133: that

PS12, Line 2135: much

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 
could fail is if the propagated timestamp +1 fails this test or the clean 
timestamp fails this test. Is that the purpose of this? Should we be trying to 
check this before we update the clock? I'm actually not sure what the right 
answer here is but intuitively it seems we should try to perform as much 
validation as possible before making changes to the system.

PS12, Line 2170: VerifyNotAncientHistory
same here; is there some way to check this before updating the clock, or we 
don't really care one way or the other?

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: Tue, 13 Feb 2018 23:55:39 +0000
Gerrit-HasComments: Yes

Reply via email to