Todd Lipcon has posted comments on this change.
Change subject: Integrate the result tracker with writes
Patch Set 17:
Here a bunch of comments I had pending on PS1 (starting to review again now the
latest patch set, but figured I'd publish these, even if they are irrelevant
Line 12: - Support for caching responses for follower transactions.
is there any support for eviction yet? if not, does this patch make it
off-by-default? otherwise this is just a memory leak?
Line 304: replica->proxy()->WriteAsync(req_, &resp_,
worth a note why this is duplicated in the WriteRequestPB
Line 243: CHECK(!overflow);
since this is a thread, we can't use FAIL(). Maybe we can just use
FlushSessionOrDie() in this code path instead?
PS1, Line 71: y exist.
whats "for the second time"?
PS1, Line 108: met
also seems kind of funky to have a random hex ID here..
Line 110: scoped_refptr<rpc::ResultTracker>(),
can we get some follow-up JIRAs filed for some of these tests? eg a test here
which validates that the bootstrap populates the cache
Line 1177: // Set up the ConsensusBootstrapInfo structure for the caller.
hacking this into this method seems like the wrong place (it's more of a
read-only method, but now it has a weird side effect)
even if we have to loop over the ops a second time I think clarity of code is
better than trying to consolidate the loops
this seems like it will slow down bootstrap noticeably... can we short-circuit
this based on the time retention? (feel free to add a TODO)
Line 1234: tablet_->StartApplying(&tx_state);
weird, shouldn't this be after it actually applies?
also, we seem to have lost the timestamp in the WriteResponsePB. I'm worried
we'll lose other fields in the future if we aren't really careful about this.
eg as a thought exercise: what if we had a 'getAndIncrement' write type with
associated results in the WriteResponsePB. How would those end up tracked?
Line 133: // a) --------- b)
I think this diagram would be clearer if the two weren't horizontally next to
each other... because this looks like it's actually a top-to-bottom sequence
diagram with two threads 'a' and 'b' instead of two different diagrams
Line 123: const char* WRITE_METHOD_NAME = "Write";
what's this used for? dont see it referenced elsewhere
To view, visit http://gerrit.cloudera.org:8080/3449
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>