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

Removed metastore -> controlplane event flow via the EventBroker. #3880

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

fulmicoton
Copy link
Contributor

@fulmicoton fulmicoton commented Sep 27, 2023

Refactoring of the control plane.

The two internal actors are replaced by two internal states.

Their execution is not concurrent anymore,
but we the code and the flow is simplified and we avoid some race conditions.

The control plane is not getting the update notify signal through the
broker anymore.

The Notify RPC is entirely removed.

Most of the test in the indexer scheduler become Control plane tests.

@fulmicoton fulmicoton marked this pull request as draft September 28, 2023 01:16
@fulmicoton fulmicoton force-pushed the complete_control_plane_metastore branch 3 times, most recently from 2c24467 to 7bd76eb Compare October 4, 2023 08:55
@fulmicoton fulmicoton force-pushed the complete_control_plane_metastore branch 2 times, most recently from ba4f319 to 6ec7a6c Compare October 4, 2023 09:09
The two internal actors are replaced by two internal states.

Their execution is not concurrent anymore,
but we the code and the flow is simplified and we avoid some race conditions.

The control plane is not getting the update notify signal through the
broker anymore.

The Notify RPC is entirely removed.

Most of the test in the indexer scheduler become Control plane tests.
@fulmicoton fulmicoton force-pushed the complete_control_plane_metastore branch from 6ec7a6c to 05ee6ed Compare October 4, 2023 09:17
@fulmicoton fulmicoton marked this pull request as ready for review October 4, 2023 09:18
Copy link
Member

@guilload guilload left a comment

Choose a reason for hiding this comment

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

Approved, but let's replug the delete source thingy.

Ok(Ok(response))
Ok(self
.ingest_controller
.get_open_shards(request, ctx.progress())
Copy link
Member

Choose a reason for hiding this comment

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

I've renamed this event GetOrCreateOpenShardsRequest to make it clear that it may create new shards. When it does, the indexing scheduler should be notified.

/// Subscribes to various metastore events and forwards them to the control plane using the inner
/// client. The actual subscriptions are set up in `quickwit-serve`.
#[derive(Debug, Clone)]
pub struct ControlPlaneEventSubscriber(ControlPlaneServiceClient);
Copy link
Member

Choose a reason for hiding this comment

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

Arg. I'm adding two callbacks in my PR :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can readd whatever is needed.

@fulmicoton
Copy link
Contributor Author

Approved, but let's replug the delete source thingy.

I think it was never plugged.

@fulmicoton fulmicoton force-pushed the complete_control_plane_metastore branch from cdd98ea to 9b8ce21 Compare October 5, 2023 02:48
@fulmicoton fulmicoton enabled auto-merge (squash) October 5, 2023 03:01
@fulmicoton fulmicoton merged commit ce0ac1f into main Oct 5, 2023
4 checks passed
@fulmicoton fulmicoton deleted the complete_control_plane_metastore branch October 5, 2023 03:22
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.

2 participants