Adar Dembo has posted comments on this change.
Change subject: log: Mark allocation finished even if allocation had an error
Patch Set 1:
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?
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
If you want to guarantee it runs no matter when it returns, you can use a
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>
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
To view, visit http://gerrit.cloudera.org:8080/3234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
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>