Todd Lipcon has posted comments on this change. Change subject: WIP: KUDU-798 (part 5) Safe time advancement in the absense of writes ......................................................................
Patch Set 21: (29 comments) mostly small stuff http://gerrit.cloudera.org:8080/#/c/5240/21//COMMIT_MSG Commit Message: Line 7: WIP: KUDU-798 (part 5) Safe time advancement in the absense of writes nit: absence PS21, Line 19: if there aren't any writes in-flight that : it doesn't know about yet I think this merits further explanation PS21, Line 22: Mvcc's safe time is a more conservative version of "global" : safe time should we consider renaming this to 'trimmable time' or 'safetime_lower_bound' or something? http://gerrit.cloudera.org:8080/#/c/5240/21/java/kudu-client/src/test/java/org/apache/kudu/client/TestHybridTime.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestHybridTime.java: PS21, Line 58: cluster nit: comma here (http://grammar.ccc.commnet.edu/GRAMMAR/commas_intro.htm has some good examples) PS21, Line 59: se absence PS21, Line 59: we expect precise clock values in the test not understanding this exactly. What does one thing have to do with the other? safetime advancements end up pushing the clock value ahead or something? Please elaborate, especially since people editing the Java client tests may not be as familiar with the internal details. PS21, Line 61: --disable_ I think 'negative' gflags are kind of confusing, because you get these kind of double-negatives. why not have it be an 'enable' flag, defaulted to true, and set it to false to disable it? http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/consensus/consensus.h File src/kudu/consensus/consensus.h: Line 28: #include "kudu/consensus/time_manager.h" can you forward decl? http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/consensus/consensus_queue-test.cc File src/kudu/consensus/consensus_queue-test.cc: Line 78: scoped_refptr<TimeManager> time_manager(new TimeManager(clock, Timestamp::kMin)); missing include for this? (I guess it's getting it transitively) http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 105: const scoped_refptr<TimeManager>& time_manager, pass by value and then use std::move below Line 275: RETURN_NOT_OK(time_manager_->AdvanceSafeTimeWithMessage(*msg->get())); above comment should be updated to reference this. would it be more efficient (and just as useful) to just do it once at the end for msgs.back()? PS21, Line 442: comma PS21, Line 443: else { : if collapse http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: Line 34: #include "kudu/consensus/time_manager.h" is forward decl sufficient? http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 533: RETURN_NOT_OK(time_manager_->AssignTimestamp(replicate)); PREPEND PS21, Line 1198: *(*iter)->get() is **iter sufficient? Can't remember under what circumstances this returns non-OK, but are we actually safe to return here rather than break? PS21, Line 1202: d comma Line 1205: RETURN_NOT_OK(time_manager_->AdvanceSafeTime(Timestamp(request->safe_timestamp()))); same, is this safe to return or should it be a CHECK? http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/consensus/raft_consensus_state.h File src/kudu/consensus/raft_consensus_state.h: Line 33: #include "kudu/consensus/time_manager.h" fwd decl? PS21, Line 256: explicit dont need explicit http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: PS21, Line 1195: the clock values are unexpected. not quite following this http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: Line 508: // Commit tx 2 - thread should still wai. typo Line 510: mgr.CommitTransaction(tx2); probably want an ASSERT_FALSE(HasResultSnapshot()) here. http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: Line 285: bool MvccManager::AnyInFlightAtOrBeforeUnlocked(Timestamp ts) const { hrm, could this just be implemented by looking at earliest_in_flight_? http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: Line 224: // time received from the leader (which can go back without leader leases). perhaps elaborate that this would crash if you tried to move it backwards? (would it?) Maybe it should be called something like SetNewTransactionLowerBound? per your commit message, the MvccManager doesn't really track safetime anymore, it just tracks a lower bound on new transactions (which itself is a lower bound on safe time). Or am I misunderstanding? Line 235: // them to complete before returning. can you clarify in the docs whether this has any bearing on safetime? i.e this does (or doesnt) guarantee that a new transaction could still start with a timestamp < timestamp http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/tablet/tablet_peer.cc File src/kudu/tablet/tablet_peer.cc: PS21, Line 168: GetCleanTimestamp( is this ever relevant at this point? isn't the tablet not bootstrapped yet? http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/tablet/transactions/write_transaction.h File src/kudu/tablet/transactions/write_transaction.h: PS21, Line 103: This must be called exactly once, during the PREPARE phase just : // after the MvccManager has assigned a timestamp. : // This also copies the timestamp from the MVCC transaction into the : // WriteTransactionState object. needs an update http://gerrit.cloudera.org:8080/#/c/5240/21/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: PS21, Line 85: unsafe is this really unsafe? or just experimental? -- To view, visit http://gerrit.cloudera.org:8080/5240 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e Gerrit-PatchSet: 21 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
