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

Make running grpc api configurable #1193

Merged
merged 7 commits into from
Dec 17, 2024
Merged

Make running grpc api configurable #1193

merged 7 commits into from
Dec 17, 2024

Conversation

afsalthaj
Copy link
Contributor

Also made the service creation to a separate function for better readability

@afsalthaj afsalthaj marked this pull request as ready for review December 15, 2024 08:20
Copy link
Contributor

@vigoo vigoo left a comment

Choose a reason for hiding this comment

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

This does not seem right to me - if the gRPC API is disabled, it does not create any of the internal services; but the goal is to have everything properly initialized (the result of create_services) and just don't run the gRPC server itself.

@vigoo
Copy link
Contributor

vigoo commented Dec 17, 2024

So now this implements what the PR says (and what I hinted in the related task) but it won't really allow us to build an alternative server API on top of it.

Basically it has the following issues:

  • We are creating this WorkerExecutorImpl struct - but that's actually the type implementing the gRPC API which we wanted to make optional. So creating it and then not starting a gRPC API does not make much sense.
  • When we will build another API that replaces the worker executor gRPC API we will need access to all the services created in the boostrap code - but this is not possible now - it just "hides" them in the WorkerExecutorImpl and then that's not accessible in any way.
  • We want the run function of boostrap to not start the default gRPC API - this is achieved - but also we will need to start our own without duplicating the whole boostrapping logic.

So I propose the following implementation instead of the one in the PR:

  • We don't need the run_grpc_server method
  • Instead add a run_server method that takes All<Ctx> as a parameter and its default implementation in the trait starts the default gRPC server, but by implementing it in the new service, we will be able to overwrite it and start our own server while still having access to All the dependencies we created.

@vigoo vigoo merged commit cf6a22f into main Dec 17, 2024
19 checks passed
@vigoo vigoo deleted the oss_debugging_service branch December 17, 2024 10:59
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