Refactoring: introduce Distributor.sendWriteRequestToBackends() #7877
Conversation
| remoteRequestContext := func() context.Context { | ||
| ctx, _ := remoteRequestContextAndCancel() | ||
| return ctx | ||
| } |
There was a problem hiding this comment.
Opinionated: I'm not sure this is really adding value.
In batchCleanup below you want the cancel, and you just call _, cancel := remoteRequestContextAndCancel() which is fine. What's the reason for not doing localCtx, _ := remoteRequestContextAndCancel() in the push function? Is it to make sure that nobody adds the cancellation call there?
There was a problem hiding this comment.
@pstibrany I agree with Oleg, but what are your thoughts? You added this function in #7871.
There was a problem hiding this comment.
It was to simplify the code and remove comment saying that we don't cancel here, only use the context. It makes more sense in "full" PR #7871, I'd keep it since we know more changes are coming.
There was a problem hiding this comment.
It was to simplify the code and remove comment saying that we don't cancel here, only use the context.
Specifically this comment:
// Do not cancel the remoteRequestContext in this callback:
// there are more callbacks using it at the same time.
which is still in this PR :)
There was a problem hiding this comment.
👍 (I imagined the reason, but I found it strange that the other usage of remoteRequestContextAndCancel does just use the underscore. Anyway, I'm also good with keeping it)
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
refactoring-before-add-write-tee-support-in-distributor
branch
from
d588529 to
d646932
Compare
pracucci
deleted the
refactoring-before-add-write-tee-support-in-distributor
branch
What this PR does
I'm working to add support to tee writes both to ingesters and partitions, as part of the migration procedure to the new experimental ingest storage. The full implementation is in this PR: #7871
In this PR I'm proposing a preliminary refactoring to make #7871 easier (and possibly safer) to review. My n. 1 goal is to not introduce any regression in the path writing to ingesters.
If you review only this PR, the design may look weird. I suggest to review the design in #7871 (and leave any comment about the design there), and treat this PR just as a preliminary PR towards that design.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.