Alexey Serbin 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: (18 comments) http://gerrit.cloudera.org:8080/#/c/7221/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7221/16//COMMIT_MSG@27 PS16, Line 27: pehaps perhaps http://gerrit.cloudera.org:8080/#/c/7221/16//COMMIT_MSG@43 PS16, Line 43: t readability nit: T http://gerrit.cloudera.org:8080/#/c/7221/16//COMMIT_MSG@44 PS16, Line 44: t1 < t readability nit: T1 < T http://gerrit.cloudera.org:8080/#/c/7221/16//COMMIT_MSG@45 PS16, Line 45: t readability nit: T http://gerrit.cloudera.org:8080/#/c/7221/16//COMMIT_MSG@74 PS16, Line 74: aditional additional http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus-test-util.h File src/kudu/consensus/consensus-test-util.h: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus-test-util.h@64 PS17, Line 64: using log::Log; : using strings::Substitute; nit: usually it's not a good idea to have 'using' in header files, unless that's inside function body or other locally enclosed scope. http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus-test-util.h@67 PS17, Line 67: inline gscoped_ptr<ReplicateMsg> CreateDummyReplicateWithOpId As I see, the pattern is always CreateDummyReplicateWithOpId().release(). Why to return a gscoped_ptr at all? Maybe, just return a raw pointer instead. http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus-test-util.h@82 PS17, Line 82: inline gscoped_ptr<ReplicateMsg> CreateDummyReplicateWithoutOpId(int64_t payload_size) { ditto: maybe, just return a raw pointer. http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus-test-util.h@115 PS17, Line 115: AppendMode mode = APPEND_NON_LEADER, Instead, is it possible to rely on the mode of the object pointed to by the queue parameter? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus-test-util.h@133 PS17, Line 133: an extra space http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_meta.cc@153 PS17, Line 153: CHECK(config.has_opid_index()); Maybe, just use DCHECK() here? As I understand, the committed configuration is verified by VerifyRaftConfig() upon flushing the data into the disk. http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.h@238 PS17, Line 238: Status LeaderAppendOperation(const ReplicateRefPtr& msg); : : Status NonLeaderAppendOperation(const ReplicateRefPtr& msg); Is it possible just to rely on the status of the queue and have a single method here? Or you wanted it exactly like this because this way it's easier to enforce some constraints? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS17: Does it make sense to add some specific test for this new behavior or it's already enough coverage? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@2622 PS17, Line 2622: RaftConfigPB This could be const reference as well. http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@2623 PS17, Line 2623: CHECK(differencer.Equals(new_config, pending_config)) << "The pending config and" nit: add a space after 'and' http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@2624 PS17, Line 2624: . nit: replace with colon? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@2844 PS17, Line 2844: // We didn't know the op id index at the time we set the config as pending so set it : // so that we can still compare the configs. : pending_config.set_opid_index(config_to_commit.opid_index()); Since this is just for comparing configs in the if() closure below, move it in there? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/time_manager-test.cc File src/kudu/consensus/time_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/time_manager-test.cc@a176 PS17, Line 176: Maybe, add an assertion instead: ASSERT_GT(time_manager_->GetSafeTime(), safe_before); -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 13 Feb 2018 16:49:01 +0000 Gerrit-HasComments: Yes
