Todd Lipcon 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/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.h@231 PS17, Line 231: // Appends a single message to be replicated to the peers. update this to indicate it also does assignment of OpId and Timestamp http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.cc@361 PS17, Line 361: op_id->CopyFrom(GetNextOpIdUnlocked()); : RETURN_NOT_OK(time_manager_->AssignTimestamp(msg->get())); per comment below about GetNextOpId having a side effect, here's an example of why I think it's dangerous. Here AssignTimestamp might fail, but then the side effect of GetNextOpId still took effect. http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.cc@421 PS17, Line 421: { nit: indent http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/consensus_queue.cc@440 PS17, Line 440: if (PREDICT_FALSE(queue_state_.first_index_in_current_term == boost::none)) { : queue_state_.first_index_in_current_term = id.index(); : } hrm, this seems a little surprising that GetNextOpId has this side effect rather than setting it on append. 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 config change. Maybe if we name it something like 'SnoopForConfigChangeUnlocked' or 'HandleConfigChangeIfNecessaryUnlocked' it would be clearer? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@2609 PS17, Line 2609: RaftConfigPB this and new_config can be const refs http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@2633 PS17, Line 2633: LOG_WITH_PREFIX_UNLOCKED(INFO) << "Skipping abort of non-pending config change. Status: " : << status.ToString(); it should still have an opid right? ie if we get to this path it should have been in the queue and been assigned an op? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/time_manager.cc File src/kudu/consensus/time_manager.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/time_manager.cc@110 PS17, Line 110: // NOTE: Currently this method just updates the clock and stores the message's timestamp. : // It always returns Status::OK() if the clock returns an OK status on Update(). hrm, is this method still even very useful? Maybe it would be clearer to just use clock->Update? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/time_manager.cc@294 PS17, Line 294: case LEADER: GetSerialTimestampUnlocked(); : FALLTHROUGH_INTENDED; this looks wrong. you are falling through to the next case which breaks anyway. Also seems odd to be calling GetSerialTimestamp without doing anything with the result. Wouldn't this function be equivalent to just say 'return GetSerialTimestampUnlocked'? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/time_manager.cc@307 PS17, Line 307: DCHECK(lock_.is_locked()); can you DCHECK_EQ(mode_, LEADER) here? -- 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: Sat, 10 Feb 2018 02:14:38 +0000 Gerrit-HasComments: Yes
