Split write request at field boundary #8167
Conversation
…ger than max allowed size Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
| return marshalWriteRequestsToRecords(tenantID, subrequests) | ||
| } | ||
|
|
||
| func marshalWriteRequestsToRecords(tenantID string, reqs [][]byte) ([]*kgo.Record, int, error) { |
There was a problem hiding this comment.
nitpick on naming: this doesn't marshal the requests it just creates records from slices of bytes
| var ( | ||
| remaining = atomic.NewInt64(int64(len(records))) | ||
| done = make(chan struct{}) | ||
| firstErrMx sync.Mutex |
There was a problem hiding this comment.
nit: you may be able to use atomic.Error here instead. Or a channel of errors
| // into subrequests with given max size. | ||
| // | ||
| // This function partially parses WriteRequest and splits the request at field boundaries. | ||
| // Some fields (source, skipLabelNameValidation) are copied into each returned subrequests. |
There was a problem hiding this comment.
i wonder if we don't need some protection against adding new fields in the WriteRequest which won't be copied over. Something like fuzzing the WriteRequest and checking if the split request contains everything but the Timeseries and Metadata. Or maybe just a test which fails if there are new fields in WriteRequest; that will force whoever adds the fields to also look at this function. WDYT?
| func putUvarintWithExpectedLength(buf []byte, val uint64, expLength int) int { | ||
| n := binary.PutUvarint(buf, val) | ||
| if n != expLength { | ||
| panic(fmt.Sprintf("expected to write %d bytes, got %d", expLength, n)) |
There was a problem hiding this comment.
the Sprintf makes the function just complex enough so it's not inlined. I'm not sure if it's worth simplifying at this point
There was a problem hiding this comment.
the Sprintf makes the function just complex enough so it's not inlined.
curious how did you find that?
There was a problem hiding this comment.
i have this external tool in GoLand
the source of go-escape-analysis.sh is this
PKG_PATH=$1
FILE_NAME=$2
FILEPATH_WIDTH="$( echo -n "$PKG_PATH/$FILE_NAME" | wc -c )"
KEY_START=$(( $FILEPATH_WIDTH + 2 ))
go build -gcflags='-m=2' "./$PKG_PATH" 2>&1 \
| grep "$PKG_PATH/$FILE_NAME" \
| sort -n -k1.$KEY_START -sthen the output looks like this
credit to @colega for sharing this
There was a problem hiding this comment.
nice :) Thanks for sharing!
pkg/mimirpb/split.go
Outdated
| if decodedLength <= 0 || decodedLength > math.MaxInt32 { | ||
| return nil, fmt.Errorf("invalid decoded length: %d", decodedLength) | ||
| } | ||
| if len(writeRequest) < int(decodedLength) { |
There was a problem hiding this comment.
here do we want to check if the rest of the buffer is smaller than decodedLength? The existing if includes the tag size and the length size
There was a problem hiding this comment.
thanks for catching this, added test.
|
|
||
| if extraBytes > 0 { | ||
| for ix := range subrequests { | ||
| // Clone subrequest before appending bytes to it. Here we could use a buffer pool. |
There was a problem hiding this comment.
i'm not super sure about this. It will double allocations right when we're receiving large requests in the first place.
Instead when we are marshalling the original request can we provide a larger slice: req.MarshalToSizedBuffer(). And then at every new subrequest we'd leave enough capacity in the slice to fit in one source and one skipLabelName.... When we're done splitting the requests we go back over the slice of subrequests and append the extra bytes if they are necessary. This way we still do the 2x copying, but at least save on the allocations.
Am I overcomplicating it?
There was a problem hiding this comment.
To put it another way - how much cheaper is to do this binary hacks+double allocation+copying vs just creating multiple WriteRequests and calling Marshall() on each of them?
There was a problem hiding this comment.
Copied from Slack: Note that remote_write clients only ever writes field=1 or 3. source is our internal field, and we only set it to non-default values from rulers (default value, API=0, is NOT serialized). Similarly, skipLabelNameValidation is our internal field, set by our enterprise proxies. That means that in vast majority of cases, this extra copying will never happen.
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
This PR implements splitting of WriteRequest by parsing marshalled WriteRequest and splitting it at field boundary.
This is alternative to #8077, implementing idea from comment #8077 (review).
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.