Adar Dembo has posted comments on this change.

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


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3234/1//COMMIT_MSG
Commit Message:

Line 14: Since this is a bug fix, it seemed cleaner to separate it
       : out into its own commit.
Really? I think we generally prefer that bugfixes be merged along with their 
tests. Without that, you can't say something like "Without the bug fix, this 
test failed 100% of the time. With the fix, it didn't fail once in 1000 runs" 
in the commit description. And it's also harder to review the test to make sure 
it's covering the right material.

Could you move the test into this commit?


http://gerrit.cloudera.org:8080/#/c/3234/1/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

Line 848
Hmm, I actually think it makes more sense to continue setting allocation_state_ 
here than in SegmentAllocationTask(). After all, it's only by calling 
PreAllocateNewSegment() that we know for sure that allocation has finished; 
suppose we have callers to PreAllocateNewSegment() in the future that bypass 
SegmentAllocationTask()?

If you want to guarantee it runs no matter when it returns, you can use a 
ScopedCleanup.


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 
equivalent to LOG(DFATAL), no?


Line 249:     boost::lock_guard<boost::shared_mutex> 
lock_guard(allocation_lock_);
Nit: given the super short scope of the lock guard, I think a variable name 
like 'l' is good; it's terse, making the code easier to read and emphasizes the 
short scope.


-- 
To view, visit http://gerrit.cloudera.org:8080/3234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If22bf946a42d0ec32c35164acd9e6e6cef18dcc3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to