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 <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, 13 Feb 2018 16:49:01 +0000
Gerrit-HasComments: Yes

Reply via email to