Adar Dembo has posted comments on this change.

Change subject: log: Mark allocation finished even if allocation had an error

Patch Set 1:

(1 comment)
File src/kudu/consensus/

Line 206:       LOG(FATAL) << "Error syncing log" << s.ToString();
> Why isn't this one DFATAL? The old code was DLOG(FATAL), which I think is e
We talked about this on Slack. I now understand that your goal was to induce a 
crash here (and presumably above) in the event that appending or syncing fails.

I strongly disagree with this. I empathize with the use case (that a leader 
replica that cannot write to its WAL should be killed), but this isn't the 
right layer for that. If we need to induce a crash when a leader can't write to 
its WAL, we should do that somewhere in the consensus module where we've 
checked that the replica is a leader and not a follower.

In general, we're doing a poor job of isolating Raft consensus details to the 
consensus module. They're spilling out all over the codebase and it's making 
Kudu as a project more difficult to maintain. I'm not trying to pick on you for 
this; obviously we're all to blame. But let's think very carefully before we 
introduce more logic that brings a Raft detail into a module where it 
(ostensibly) doesn't belong.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If22bf946a42d0ec32c35164acd9e6e6cef18dcc3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to