Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Leader election slos #4974

Merged
merged 14 commits into from
Sep 9, 2020
Merged

Leader election slos #4974

merged 14 commits into from
Sep 9, 2020

Conversation

sudiksha27
Copy link
Contributor

@sudiksha27 sudiksha27 commented Sep 4, 2020

Goals (and why):
Fix bug with leader election rate health check. The health check should be analyzing the metrics per client.

Implementation Description (bullets):

  • Register client wise metrics to leader election rate health check.

  • Leader election health check performs analysis on the sum of rates of all clients

Testing (What was existing testing like? What have you done to improve it?):
Tested on internal testing environment

Concerns (what feedback would you like?):
Does the wiring make sense?

Where should we start reviewing?:
LeaderElectionHealthCheck.java, TimeLockAgent.java

Priority (whenever / two weeks / yesterday):
As soon as possible 🔥

@changelog-app
Copy link

changelog-app bot commented Sep 4, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

The leader election rate health check now depends on metrics registered per client. Earlier, the health check was analyzing incorrect metrics.

Check the box to generate changelog(s)

  • Generate changelog entry

@sudiksha27 sudiksha27 marked this pull request as ready for review September 7, 2020 11:30
@gmaretic gmaretic self-requested a review September 8, 2020 10:32
Copy link
Contributor

@gmaretic gmaretic left a comment

Choose a reason for hiding this comment

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

Looks fine, a few nits and would like a test for multiple clients though

@@ -0,0 +1,6 @@
type: improvement
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fix

private static final double MAX_ALLOWED_LAST_5_MINUTE_RATE = 0.016;
private static final double MAX_ALLOWED_LAST_5_MINUTE_RATE = 0.015;
private final ConcurrentMap<Client, LeaderElectionServiceMetrics> clientWiseMetrics
= new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: one line, and extra newline


private final LeaderElectionServiceMetrics leaderElectionServiceMetrics;
public void registerClient(Client namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: one line

private final LeaderElectionHealthCheck leaderElectionHealthCheck =
new LeaderElectionHealthCheck(leaderElectionServiceMetrics);
private final LeaderElectionHealthCheck leaderElectionHealthCheck = new LeaderElectionHealthCheck();
private static final Client CLIENT = Client.of("abc");
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test with multiple clients

@gmaretic gmaretic self-requested a review September 9, 2020 11:52
@gmaretic
Copy link
Contributor

gmaretic commented Sep 9, 2020

LGTM!

@sudiksha27 sudiksha27 merged commit 0a6c4e1 into develop Sep 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the leader-election-slos branch September 9, 2020 13:20
@svc-autorelease
Copy link
Collaborator

Released 0.245.2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants