[index] Prevent duplicated writes to commit log #1375
Conversation
arnikola
changed the title
arnikola
added
area:index
arnikola
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1375 +/- ##
=========================================
+ Coverage 70.7% 71.4% +0.6%
=========================================
Files 827 48 -779
Lines 71229 4239 -66990
=========================================
- Hits 50398 3027 -47371
+ Misses 17534 999 -16535
+ Partials 3297 213 -3084
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1375 +/- ##
======================================
Coverage 70.8% 70.8%
======================================
Files 833 833
Lines 71487 71487
======================================
Hits 50642 50642
Misses 17533 17533
Partials 3312 3312
Continue to review full report at Codecov.
|
src/dbnode/storage/shard.go
Outdated
| ) | ||
| if writable { | ||
| // Perform write | ||
| err = entry.Series.Write(ctx, timestamp, value, unit, annotation) | ||
| shouldWrite, err = entry.Series.Write(ctx, timestamp, value, unit, annotation) | ||
| if err != nil { |
There was a problem hiding this comment.
Any reason you moved the error handling code around? it looks like the old code only had to call DecrementReaderWriterCount once but this one has to do it multiple times
There was a problem hiding this comment.
So this should still call DecrementReaderCount once, depending on which operations error; the previous code would still do these calls:
commitLogSeriesID = entry.Series.ID()
commitLogSeriesTags = entry.Series.Tags()
commitLogSeriesUniqueIndex = entry.Index
for no reason if .Write(..) errored. Happy to revert it though if this is fine
There was a problem hiding this comment.
yeah I think i'd prefer not to change this for now as I think its a little more brittle now (cleanup code more spread out)
src/dbnode/storage/database.go
Outdated
| // whose lifecycle lives longer than the span of this request, making them | ||
| // safe for use by the async commitlog. Need to set the outcome in the | ||
| // error case so that the commitlog knows to skip this entry. | ||
| writes.SetOutcome(i, series, err) |
There was a problem hiding this comment.
Not sure if I brought this up last time, but is there any reason we can't just SetOutcome in both cases? Feels a little safer to make sure everything gets updated anyways and I'm pretty sure its cheap since it literally just does this
func (b *writeBatch) SetOutcome(idx int, series Series, err error) {
b.writes[idx].Write.Series = series
b.writes[idx].Err = err
}There was a problem hiding this comment.
So basically always SetOutcome
and then
if !wasWritten || err != nil {
writes.SetSkipWrite(i)
}There was a problem hiding this comment.
Notice that I changed it to an OR because you always want to skip the write if it wasn't written or there was an error. I know the commitlog will skip it anyways if there was an error but this seems more semantically correct
There was a problem hiding this comment.
Sure, sounds good to me; makes sense that the readability is worth more here
Propagates a
shouldWriteflag from the buffer if a point with the same value at the same time exists, which determines if the point should be added to the commit log or not.