Todd Lipcon has posted comments on this change. Change subject: KUDU-798 (part 5) Correct safe time advancement ......................................................................
Patch Set 27: (10 comments) http://gerrit.cloudera.org:8080/#/c/5240/27//COMMIT_MSG Commit Message: PS27, Line 15: it's its http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 450: // If we're not sending ops to the follower, set the safe time on the request. worth a TODO here that we could also set it if we are sending our latest op to the follower? http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: Line 34: //#include "kudu/consensus/time_manager.h" nit http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS27, Line 1198: fail : // once we have that functionality we'll have to revisit this. nit: two sentences Line 1204: // All transactions that are going to be prepared were started, advance the safe timestamp. this doesn't smell quite right: imagine the leader sent: 1.1 @ time 10 1.2 @ time 20 1.3 @ time 30 safetime = 40 and then we failed to prepare 1.3.. in that case, we can't go ahead and set safetime to 40 http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 37: #include "kudu/consensus/time_manager.h" needed? http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/tablet/tablet_peer_mm_ops.cc File src/kudu/tablet/tablet_peer_mm_ops.cc: Line 26: #include "kudu/consensus/time_manager.h" unnecessary http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/tablet/transactions/alter_schema_transaction.cc File src/kudu/tablet/transactions/alter_schema_transaction.cc: Line 23: #include "kudu/consensus/time_manager.h" unnecessary http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/tablet/transactions/transaction_tracker.cc File src/kudu/tablet/transactions/transaction_tracker.cc: Line 24: #include "kudu/consensus/time_manager.h" unnecessary http://gerrit.cloudera.org:8080/#/c/5240/27/src/kudu/tablet/transactions/write_transaction.cc File src/kudu/tablet/transactions/write_transaction.cc: Line 25: #include "kudu/consensus/time_manager.h" same -- 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: 27 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
