[dbnode] Use already encoded tags when writing time series to commit log #1898
Conversation
…o r/fast-encode-tags
| @@ -28,7 +28,7 @@ import: | |||
| version: ^0.8 | |||
|
|
|||
| - package: github.com/apache/thrift | |||
| version: 0.9.3-pool-read-binary-2 | |||
| version: 0.9.3-pool-read-binary-3 | |||
There was a problem hiding this comment.
Can we just merge this into master of our branch? Pretty confusing that we're pinned to a branch
There was a problem hiding this comment.
Not opposed to it, problem is master is way ahead of where we are at, so we'd have to reset master all the way back to 0.9.3 on our fork which would be weird...
| @@ -252,6 +253,9 @@ type PoolingPolicy struct { | |||
| // The initial alloc size for a block. | |||
| BlockAllocSize *int `yaml:"blockAllocSize"` | |||
|
|
|||
| // The thrift bytes pool max bytes slice allocation for a single binary field. | |||
| ThriftBytesPoolMaxAllocSize *int `yaml:"thriftBytesPoolMaxAllocSize"` | |||
There was a problem hiding this comment.
Its not really a max is it right? its always this size?
There was a problem hiding this comment.
The capacity is always this size, but the actual length/size is determined by the length of the bytes being copied.
Since we suffix it with "Alloc" I'm not opposed to just calling it AllocSize and dropping the Max.
| @@ -1764,7 +1769,11 @@ func (r *writeBatchPooledReq) Finalize() { | |||
| if r.writeTaggedReq != nil { | |||
| for _, elem := range r.writeTaggedReq.Elements { | |||
| apachethrift.BytesPoolPut(elem.ID) | |||
| apachethrift.BytesPoolPut(elem.EncodedTags) | |||
| // Ownership of the encoded tagts has been transferred to the BatchWriter | |||
| @@ -72,9 +76,13 @@ type Series struct { | |||
| // ID is the series identifier. | |||
| ID ident.ID | |||
|
|
|||
| // Tags are the series tags. | |||
| // Tags is the series tags. | |||
| Tags ident.Tags | |||
There was a problem hiding this comment.
Is this still used anywhere?
There was a problem hiding this comment.
Yeah, it's used on the read side and it was going to be very difficult to remove unfortunately.
src/dbnode/ts/write_batch.go
Outdated
| "time" | ||
|
|
||
| "github.com/m3db/m3/src/x/ident" | ||
| xtime "github.com/m3db/m3/src/x/time" | ||
| ) | ||
|
|
||
| var ( | ||
| errTagsAndEncodedTagsRequired = errors.New("tags iterator and encoded tags required to be provided") |
There was a problem hiding this comment.
"required to be provided" sounds funny. Maybe: "must be provided"
Codecov Report
@@ Coverage Diff @@
## master #1898 +/- ##
=========================================
+ Coverage 32.1% 65.7% +33.6%
=========================================
Files 4 773 +769
Lines 342 71560 +71218
=========================================
+ Hits 110 47081 +46971
- Misses 222 20924 +20702
- Partials 10 3555 +3545
Continue to review full report at Codecov.
|
What this PR does / why we need it:
This improves raw commit log write speed by using the encoded tags from the RPC batch rather than encoding from the newly created series tags. This is achieved by extending the lifetime of the tags from the RPC write request.
It also adds the ability to set in the pooling config a value for
thriftBytesPoolAllocSizewhich can adjust from the default 1024 value a higher or lower to provided pooled bytes to thrift binary fields. This enables tweaking for workloads with a lot of tags, in the future this should hopefully be more adaptive.Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: