Alexey Serbin has posted comments on this change. ( )

Change subject: Simplify OpId/Timestamp assignment and make it atomic

Patch Set 17:

Commit Message:
PS16, Line 27: pehaps
PS16, Line 43: t
readability nit: T
PS16, Line 44: t1 < t
readability nit: T1 < T
PS16, Line 45: t
readability nit: T
PS16, Line 74: aditional
File src/kudu/consensus/consensus-test-util.h:
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.
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.
PS17, Line 82: inline gscoped_ptr<ReplicateMsg> 
CreateDummyReplicateWithoutOpId(int64_t payload_size) {
ditto: maybe, just return a raw pointer.
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?
PS17, Line 133:
an extra space
File src/kudu/consensus/
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.
File src/kudu/consensus/consensus_queue.h:
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?
File src/kudu/consensus/

Does it make sense to add some specific test for this new behavior or it's 
already enough coverage?
PS17, Line 2622: RaftConfigPB
This could be const reference as well.
PS17, Line 2623:       CHECK(differencer.Equals(new_config, pending_config)) << 
"The pending config and"
nit: add a space after 'and'
PS17, Line 2624: .
nit: replace with colon?
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.
Since this is just for comparing configs in the if() closure below, move it in 
File src/kudu/consensus/
PS17, Line 176:
Maybe, add an assertion instead:

ASSERT_GT(time_manager_->GetSafeTime(), safe_before);

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Tue, 13 Feb 2018 16:49:01 +0000
Gerrit-HasComments: Yes

Reply via email to