Fix annotation pooling #1476
Fix annotation pooling #1476
Conversation
richardartoul
requested review from
robskillington,
Haijuncao,
simonrobb and
prateek
Codecov Report
@@ Coverage Diff @@
## master #1476 +/- ##
=========================================
- Coverage 70.9% 61% -9.9%
=========================================
Files 841 612 -229
Lines 71895 56313 -15582
=========================================
- Hits 51000 34376 -16624
- Misses 17555 19287 +1732
+ Partials 3340 2650 -690
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1476 +/- ##
=========================================
- Coverage 70.9% 65.8% -5.2%
=========================================
Files 841 657 -184
Lines 71895 54023 -17872
=========================================
- Hits 51000 35550 -15450
+ Misses 17555 16121 -1434
+ Partials 3340 2352 -988
Continue to review full report at Codecov.
|
richardartoul
added
T: Bug
area:db
| @@ -127,4 +131,6 @@ type BatchWriter interface { | |||
| unit xtime.Unit, | |||
| annotation []byte, | |||
| ) | |||
|
|
|||
| SetFinalizeAnnotationFn(f FinalizeAnnotationFn) | |||
There was a problem hiding this comment.
how would you feel about changing FinalizeAnnotationFn to FinalizeWriteFn? That way the caller can decide what level of finalization they need (e.g. if they need to do stuff to fields beyond just the annotation)
also nit: add a comment above fn
There was a problem hiding this comment.
talked offline: going to leave as is and add a comment explaining the choice
There was a problem hiding this comment.
Spoke offline and agreed to leave as is since the lifecycle is already confusing enough without making it more generic
| if elem.Datapoint.Annotation != nil { | ||
| apachethrift.BytesPoolPut(elem.Datapoint.Annotation) | ||
| } | ||
| // Don't return the annotations to the pool because they don't |
There was a problem hiding this comment.
the first sentence in the comment is confusing unless people understand nuances of when IDs are cloned v when retained. much simpler to just rely on the second sentence and say the ownership of the lifecycle for the annotations has been transferred to the BatchWriter.
that said, can we do the same thing for the ID/EncodedTags too? i.e. expect the BatchWriter to clean it up? might cause a minor rise in steady state memory usage but would simplify this code a lot.
There was a problem hiding this comment.
Fixed the comment. Spoke offline and decided not to do the ID/EncodedTags thing for performance reasons (more synchronization required by single-threaded commitlog)
| } | ||
|
|
||
| func TestShardWriteAsyncWithAnnotations(t *testing.T) { | ||
| testShardWriteAsync(t, []testWrite{ |
There was a problem hiding this comment.
one more test case - ensure we don't copy the annotation bytes if the series already exists in the shard
Fixes a pooling bug that was caused by returning annotations to the apache thrift byte pool once RPCs had completed. This was problematic because the commit log still had a reference to them, as well as the shard insert queue if we were inserting a new series asynchronously.