Todd Lipcon has posted comments on this change. Change subject: WIP: KUDU-798 (part 4) Add a TimeManager to manage safe time advancement ......................................................................
Patch Set 7: (20 comments) http://gerrit.cloudera.org:8080/#/c/5300/7/src/kudu/consensus/time_manager-test.cc File src/kudu/consensus/time_manager-test.cc: Line 43: // Returns a latch that allows to incomplete sentence Line 46: latches_.push_back(std::move(latch)); why not just do: latches_.emplace_back(new CountDownLatch(1)) PS7, Line 48: Thread::Create("test", "time_manager-test", &TimeManagerTest::WaitForSafeTimeThread, : this, latches_.back().get(), safe_time, &thread); : for the purposes of tests, I think just using std::thread with a lambda here is probably a little easier to follow (we don't really care about registering in threadz, etc) http://gerrit.cloudera.org:8080/#/c/5300/7/src/kudu/consensus/time_manager.cc File src/kudu/consensus/time_manager.cc: Line 51: TimeManager::TimeManager(const scoped_refptr<Clock>& clock, Timestamp initial_safe_time) nit: pass by value and use std::move Line 84: RETURN_NOT_OK(clock_->Update(Timestamp(t))); redundant cast? Line 90: last_serial_ts_assigned_ = Timestamp(message.timestamp()); is it worth a check here (either a Status-returning one or an assertion) that the last_serial_ts_assigned_ doesn't go backwards? or is it expected that it might go backwards today due to lack of leases? worth a comment either way, and if we think it's rare enough maybe a LOG(INFO) when it goes backwards PS7, Line 179: are no transactions in between assignment and start, this wording is kind of vague. i think it would be good to outline the exact scenario we're worried about here with a kind of timeline ascii art. I can sort of understand it because we talked about it recently but in a week or two I'll probably forget and not understand this either. Line 197: case LEADER: { this logic could also use a good comment http://gerrit.cloudera.org:8080/#/c/5300/7/src/kudu/consensus/time_manager.h File src/kudu/consensus/time_manager.h: Line 33: // specify thread safety (or lack thereof) PS7, Line 39: // Sets this TimeManager to leader mode, requires that GrantLease() has been : no GrantLease yet? Line 43: // Sets this TimeManager to non-leader mode. would be good to add some class-level doc about the two "modes" and what they mean. The constructor should probably indicate which "mode" it starts in. PS7, Line 46: chosen makes it sound like this is an argument. Maybe "the message's ExternalConsistencyMode" PS7, Line 50: e. A nit: comma, not new sentence Line 51: // to the leader's queue and can determine which is the safe time to send to non-leader this sentence reads a little funny... "the queue knows of the messages appended to the queue"? Line 55: Status AssignTimestamp(ReplicateMsg* message); under what circumstances does it return an error status? Line 57: // Updates the internal state based on 'message' received from a leader replica. can you clarify a bit more what effect this has, or at least that we expect that the replica calls this on every message? PS7, Line 60: Status AdvanceSafeTimeWithMessage(const ReplicateMsg& message); : : Status AdvanceSafeTime(Timestamp safe_time); docs Line 64: // Updates the current safe time, if the TimeManager is in non-leader mode. does this mean tat it's a no-op if it's in leader mode? or is it a CHECK? Line 75: Timestamp GetSafeTime() const; maybe move the implementation details into the .cc file? Line 123: // Lock to protect all the fields above. nit: usually we define the lock first, not last -- To view, visit http://gerrit.cloudera.org:8080/5300 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0bb2b1d2590ed7ead6f1980f9572be10444bb81b Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
