Don't return error on missing writable bucket during cold flush #2188
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2188 +/- ##
======================================
Coverage 66.8% 66.8%
======================================
Files 1016 1016
Lines 88298 88298
======================================
Hits 59026 59026
Misses 25070 25070
Partials 4202 4202
Continue to review full report at Codecov.
|
| // No-op if the writable bucket doesn't exist. | ||
| // This function should only get called for blocks that we know need to be | ||
| // cold flushed. However, buckets that get attempted to be cold flushed and | ||
| // fail need to get cold flushed as well. These kinds of buckets will have |
There was a problem hiding this comment.
Dumb clarification question--why is the version non writeable on the retry?
There was a problem hiding this comment.
Nah not dumb. The "writeable" bucket version is always zero. When we cold flush, we mark the writeable bucket a different version so that we know which buckets have flushed - this version stamp is done in the if block above.
However, when we change the version above, we're not actually flushed because we don't write to disk until every series in this shard is complete flushing. Therefore, if we for some reason fail to flush for some other series, this bucket that we just marked as a different version needs to be flushed again the next time we try to flush.
After a successful flush, these non-writeable buckets will get evicted from memory.
justinjc
force-pushed
the
juchan/cold-flush-bad-err
branch
from
c45e071 to
b2f9f4c
Compare
What this PR does / why we need it:
During a cold flush, we first collect all the block starts that require cold flushing. This includes two cases:
After getting all these blocks, we go through each one to fetch data and compact.
Therefore when fetching data for a ColdFlush, encountering a block that has no writable bucket is not an error, since that means we're hitting case (2).
Please see the section explaining bucket versions here for more context if needed.
Special notes for your reviewer: N/A
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: