Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7221 )
Change subject: Simplify OpId/Timestamp assignment and make it atomic ...................................................................... Patch Set 17: (10 comments) http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/quorum_util.cc@158 PS17, Line 158: std::set<string> uuids; add: DCHECK(type == COMMITTED_CONFIG || type == PENDING_CONFIG) http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.h@449 PS17, Line 449: mus must http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@648 PS17, Line 648: RETURN_NOT_OK(HandlePendingConfigChangeUnlocked(round)); > although this is right, it looks confusing because round probably isn't a c or just: if (round->replicate_msg()->op_type() == CHANGE_CONFIG_OP) { ... } http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@662 PS17, Line 662: if (PREDICT_TRUE(round->replicate_msg()->op_type() != CHANGE_CONFIG_OP)) return Status::OK(); Due to the method name, seems less surprising to add: DCHECK_EQ(CHANGE_CONFIG_OP, round->replicate_msg()->op_type()); http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@673 PS17, Line 673: nit: extra line http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@691 PS17, Line 691: ( nit: no need for inner parentheses http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc File src/kudu/tablet/transactions/transaction_driver.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@336 PS17, Line 336: rule s/rule/invariant/ http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@360 PS17, Line 360: constraint s/constraint/above invariant/ By the way: is there a DCHECK somewhere in mvcc to enforce the invariant that clean time < any tx timestamp? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@371 PS17, Line 371: repl_state_copy This variable masks a variable of the same name in an outer scope http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@384 PS17, Line 384: case REPLICATING can we still get to this case? -- To view, visit http://gerrit.cloudera.org:8080/7221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bedcc5c3708d06f48131f34ea14c835867b4800 Gerrit-Change-Number: 7221 Gerrit-PatchSet: 17 Gerrit-Owner: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 20 Feb 2018 22:20:34 +0000 Gerrit-HasComments: Yes
