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.

Sign up for GitHub

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

Jump to bottom

Block-builder-scheduler: RPC service and client module #10089

Merged
merged 49 commits into from
Dec 19, 2024
Merged

Block-builder-scheduler: RPC service and client module #10089

merged 49 commits into from
Dec 19, 2024

Conversation

seizethedave
Copy link
Contributor

@seizethedave seizethedave commented Dec 2, 2024 •
edited
Loading

What this PR does

  • Add proto stubs for the block-builder-scheduler gRPC service, and registers block-builder-scheduler as a server of this service at startup. The two RPC methods (AssignJob and UpdateJob) call into methods already implemented in previous PRs.
  • Make the job queue generic so that the spec payload can be the type defined in the gRPC stubs.
  • Add a client module (blockbuilder/schedulerpb/client.go) that encapsulates the communication protocol expected by block-builder-scheduler (where workers are expected to constantly send and re-send job updates). This type exposes a GetJob / CompleteJob interface to block-builder.
  • Improvements to logging in block-builder-scheduler after testing the Assign/Update flow under docker-compose.

dev env changes:

  • Adds mimir-block-builder and mimir-block-builder-scheduler containers to the mimir-ingest-storage docker-compose dev environment.

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.

@seizethedave
Merge from davidgrant/block-builder-scheduler-reconcile
f457910
@seizethedave
Don't need to get fancy with critical section: no contention is possi…
8f9d996 
…ble yet.
@seizethedave
Document s.committed.
6712985
@seizethedave
Merge
f97e4cb
@seizethedave
Fix merge slop.
4cfeed9
@seizethedave
Add proto defs.
eaed668
@seizethedave
Initial RPC protos.
d37b1f2
@seizethedave
Log an error if flushing fails.
35d1868
@seizethedave
Include RPC forms of assignJob/updateJob.
81dee72
@seizethedave
Merge remote-tracking branch 'origin/main' into davidgrant/block-buil…
3772a12 
…der-scheduler-kafka-flush
@seizethedave
Fleshing out the client module.
7c3f2da
@seizethedave
Appease the linter.
db44d45
@seizethedave
workerID param not used.
5cc39ce
@seizethedave
CompleteJob: no context needed.
3c6a566
@seizethedave
The linter.
91b8e83
@seizethedave
More logs.
7355de9
@seizethedave
Fix circular reference. Initialize client.
c34617d
@seizethedave
schedulerproto -> schedulerpb
9f67caa
@seizethedave
merge
47b5662
@seizethedave
Fix compile stuff.
188217f
@seizethedave
Getting builder/scheduler up in docker-compose.
179018e
@seizethedave
Clean unneeded flags.
f789447
@seizethedave
Fixes for BB<>scheduler communications:
bad7dc2 
- accidentally wasn't calling scheduler.assignJob in the RPC.
- Add logging for jobQueue operations.
- Experimentally tear into blockbuilder to begin consuming jobs from scheduler.
@seizethedave
Fix block-builder's scheduler_config so it can connect to a scheduler.
762012f
@seizethedave
Shorter update interval for testing.
671f4b3
@seizethedave
Fix unloggable key.
f760fa8
@seizethedave
Longer job working time.
df7e43e
@seizethedave
Merge remote-tracking branch 'origin/main' into davidgrant/block-buil…
6ad943c 
…der-scheduler-kafka-flush
@seizethedave
Undo blockbuilder.go hacking.
63313b0
@seizethedave
Peel off block-builder changes not ready for this PR.
509e9a8
@seizethedave seizethedave changed the title Block-builder-scheduler: RPC layer and client module Block-builder-scheduler: RPC service and client module Dec 2, 2024
@seizethedave seizethedave marked this pull request as ready for review December 2, 2024 22:48
@seizethedave seizethedave requested a review from a team as a code owner December 2, 2024 22:48
@seizethedave seizethedave requested a review from codesome December 3, 2024 18:54
codesome
codesome reviewed Dec 6, 2024
View reviewed changes
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

Mostly small comments, and I suggest updating the client_test.go a bit. Other than that, looking good! Thanks!

pkg/blockbuilder/scheduler/scheduler.go Outdated Show resolved Hide resolved
pkg/blockbuilder/schedulerpb/client.go Show resolved Hide resolved
pkg/blockbuilder/schedulerpb/client.go
return key, spec, nil
}

return JobKey{}, JobSpec{}, lastErr
Copy link
Member

Choose a reason for hiding this comment

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

If it was because of context cancellation, the error will be nil. It would be a good idea to mention in the comments of this function above that the returned job is valid iff the jobKey.Id is not empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the context is canceled, the error will be non-nil.

https://go.dev/play/p/ASExdX5QdMl

pkg/blockbuilder/schedulerpb/client.go Outdated Show resolved Hide resolved
pkg/blockbuilder/schedulerpb/client_test.go Outdated Show resolved Hide resolved
pkg/blockbuilder/schedulerpb/client_test.go Outdated Show resolved Hide resolved
pkg/blockbuilder/schedulerpb/client_test.go Show resolved Hide resolved
pkg/blockbuilder/schedulerpb/client_test.go Show resolved Hide resolved
narqo
narqo approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@narqo narqo left a comment

Choose a reason for hiding this comment

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

🔥 Works for me

pkg/blockbuilder/scheduler/scheduler.go Outdated Show resolved Hide resolved
pkg/blockbuilder/scheduler/scheduler.go Outdated Show resolved Hide resolved
@seizethedave seizethedave merged commit dc1410c into main Dec 19, 2024
29 checks passed
@seizethedave seizethedave deleted the davidgrant/block-builder-scheduler-rpc-client branch December 19, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narqo narqo narqo approved these changes

@codesome codesome Awaiting requested review from codesome

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seizethedave @narqo @codesome