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

Chore: change grpc server ownership #87148

Merged
merged 1 commit into from May 13, 2024
Merged

Conversation

ArturWierzbicki
Copy link
Contributor

What is this feature?

[Add a brief description of what the feature or update does.]

Why do we need this feature?

[Add a description of the problem the feature is trying to solve.]

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@ArturWierzbicki ArturWierzbicki added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Apr 30, 2024
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone Apr 30, 2024
@ryantxu
Copy link
Member

ryantxu commented May 2, 2024

🤔 it may be better if we just remove the generic service/library/support

The generic gRPC server was designed to allow "services" across grafana to inject contracts into a shared server via wire. This made sense before we agreed on k8s/apiservers as the standard intra-process/app communication system.

Alerting is currently in the process of shifting from using this gRPC service towards an http api managed by k8s. Once that is updated, unified storage can copy/move whatever it wants and have full control of the gRPC server rather than trying to manage a shared system we do not want people to use

@ArturWierzbicki
Copy link
Contributor Author

🤔 it may be better if we just remove the generic service/library/support

The generic gRPC server was designed to allow "services" across grafana to inject contracts into a shared server via wire. This made sense before we agreed on k8s/apiservers as the standard intra-process/app communication system.

Alerting is currently in the process of shifting from using this gRPC service towards an http api managed by k8s. Once that is updated, unified storage can copy/move whatever it wants and have full control of the gRPC server rather than trying to manage a shared system we do not want people to use

I think that's fair. So, we want to remove the grpcserver package and fold what is needed of it into the Storage API.

In this case, I still think it makes sense to transfer the ownership, as any changes between now and the time the package is removed will be driven by Storage API needs. With the ownership transferred, reviews/discussions of such changes will be scoped to just a single team -- the team that requires those changes :)

@ryantxu
Copy link
Member

ryantxu commented May 2, 2024

yes -- just want to make sure the goal is to remove it as a generic service, but the ownership proposed in this PR LGTM.

@ArturWierzbicki ArturWierzbicki requested a review from a team May 8, 2024 17:06
@inf0rmer inf0rmer merged commit e34e9f4 into main May 13, 2024
21 checks passed
@inf0rmer inf0rmer deleted the change-ownership-of-grpc-server branch May 13, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants