Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring: introduce Distributor.sendWriteRequestToPartitions() and sendWriteRequestToIngesters() #7891

Merged

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Apr 12, 2024

What this PR does

This PR is a follow up of #7877.

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 send and last 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.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

… sendWriteRequestToIngesters()

Signed-off-by: Marco Pracucci <[email protected]>
@@ -7416,46 +7438,43 @@ func TestStartFinishRequest(t *testing.T) {
}
}

func TestSendMessageMetadata(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: this was was directly calling Distributor.sendToIngester() which doesn't exist anymore. I've refactored the test to run like other tests, and then assert on the gRPC context when the mockIngester.Push() request is received.

beforePushHook func(ctx context.Context, req *mimirpb.WriteRequest) (*mimirpb.WriteResponse, error, bool)
}

func (i *mockIngester) registerBeforePushHook(fn func(ctx context.Context, req *mimirpb.WriteRequest) (*mimirpb.WriteResponse, error, bool)) {
Copy link
Collaborator Author

@pracucci pracucci Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: this will be also used by a new test that will be added by #7871.

Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci marked this pull request as ready for review April 12, 2024 10:40
@pracucci pracucci requested a review from a team as a code owner April 12, 2024 10:40
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you.

@pracucci
Copy link
Collaborator Author

I tripled checked it 🤞

@pracucci pracucci merged commit 9930b29 into main Apr 12, 2024
29 checks passed
@pracucci pracucci deleted the refactoring-before-add-write-tee-support-in-distributor branch April 12, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants