David Ribeiro Alves 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) 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 Done PS21, Line 19: if there aren't any writes in-flight that : it doesn't know about yet > I think this merits further explanation this is more of a time manager detail. explained it further there. 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_bou not sure, with leader leases it can actually be the same as the time manager's safe time.. 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 ha thanks. done PS21, Line 59: se > absence Done PS21, Line 59: we expect precise clock values in the test > not understanding this exactly. What does one thing have to do with the oth Done PS21, Line 61: --disable_ > I think 'negative' gflags are kind of confusing, because you get these kind Done 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? Done 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) Done 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 Done Line 275: RETURN_NOT_OK(time_manager_->AdvanceSafeTimeWithMessage(*msg->get())); > above comment should be updated to reference this. Done. It was not possible before, but with recent changes to the TimeManager it now is. Also we should only do this in leader mode (until we have leader leases we only advance safe time in replicas on commit to reduce opportunity for it to move back) so added that condition PS21, Line 442: > comma Done PS21, Line 443: else { : if > collapse Done 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? Done 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 I changed this to a check ok. in this case we just changed the queue/time_manager to leader mode under the consensus lock so this shouldn't fail PS21, Line 1198: *(*iter)->get() > is **iter sufficient? good point. I changed this to a CHECK_OK. Currently it only fails if we can't update the clock (in which case we should crash). If this changes in the future we'll have to update this. Left a TODO for myself. PS21, Line 1202: d > comma Done Line 1205: RETURN_NOT_OK(time_manager_->AdvanceSafeTime(Timestamp(request->safe_timestamp()))); > same, is this safe to return or should it be a CHECK? this returns void now and does a check internally. 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? Done PS21, Line 256: explicit > dont need explicit Done 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 The above is required because, in the tests that call this, we're using logical values for the timestamps. This is not required anymore. 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 Done Line 510: mgr.CommitTransaction(tx2); > probably want an ASSERT_FALSE(HasResultSnapshot()) here. Done 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_? It could, but I thought you said the current implementation was a bug? (i.e. how is this different from the method above) I reverted the rename and left a TODO(todd) 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? this won't crash as it doesn't affect correctness (submitting a committed timestamp after moving clean time would though). I tried a crash but we have a bunch of test-only code like LocalTabletWriter that might set this back. Because of this I left a LOG_EVERY_N(ERROR) but didn't make it crash. I couldn't see any crashed on the full stack itests though. Line 235: // them to complete before returning. > can you clarify in the docs whether this has any bearing on safetime? i.e t Added a note that 'timestamp' must be marked as safe first. 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? yeah, but the time manager doesn't know what is "safe" yet. this way even if the replica is partitioned out it can still serve scans for ts's that come before the last know clean time (which is a strict lower bound for safe time) 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 Done 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? increasing this would increase the likelyhood of hanging scanner threads for a long time. It was a hard clamp before. I stand by the unsafe. -- 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 <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
