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

Remove unused grpc health check methods #6113

Merged
merged 1 commit into from
May 6, 2022

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented May 6, 2022

What this PR does / why we need it:
We have generic grpc_healthcheck endpoint that get registered for all the components
during initialization stage.
https://github.com/grafana/loki/blob/main/pkg/loki/loki.go#L364

Looks like these old health checks that are part of actual component's struct itself came from legacy.

Signed-off-by: Kaviraj kavirajkanagaraj@gmail.com

NOTE: This should be no op. No build or test failure as there are no consumer for these methods

Which issue(s) this PR fixes:
Fixes # NA

Special notes for your reviewer:
We figured this out while investigating this ingester/querier grpc issue #6023

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

We have generic `grpc_healthcheck` endpoint that get registered for all the components
during initialization stage.

Looks like these health checks that are part of actual component's struct itself came from legacy.

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
@kavirajk kavirajk requested a review from a team as a code owner May 6, 2022 12:10
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0.1%
+        distributor	1%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@kavirajk kavirajk changed the title Remove unused grpc health check endpoints Remove unused grpc health check methods May 6, 2022
@kavirajk kavirajk requested a review from chaudum May 6, 2022 12:34
// Check implements grpc_health_v1.HealthCheck.
func (i *Ingester) Check(ctx context.Context, req *grpc_health_v1.HealthCheckRequest) (*grpc_health_v1.HealthCheckResponse, error) {
status := grpc_health_v1.HealthCheckResponse_SERVING
if (i.State() != services.Running) || (i.lifecycler.GetState() != ring.ACTIVE) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried that we don't seem to be checking the lifecycler state in our blanket version, but it doesn't look like these are used either.

@owen-d owen-d merged commit 24f8b19 into main May 6, 2022
@owen-d owen-d deleted the kavirajk/remove-unused-health-checkendpoint branch May 6, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants