-
Notifications
You must be signed in to change notification settings - Fork 20
Added Election Term Metric #557
base: main
Are you sure you want to change the base?
Added Election Term Metric #557
Conversation
This Metric will Publish the Only Election term number
@@ -41,6 +41,7 @@ | |||
THREAD_POOL, | |||
SHARD_STATS, | |||
MASTER_PENDING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can rename MASTER_PENDING
as MASTER_METRICS
and then election term can also be MASTER_METRICS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In metricPathmap we are making map of Metric name Metric path. So, it is not possible to make same metric name for 2 different paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we group MASTER_PENDING
and ELECTION_TERM
into one metric MASTER_METRICS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As from my understanding we want to publish two different metrics. so it is not possible to use same Metric name for two different metrics. If it is a way to use same metric name then can you give some context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved conflict
@@ -41,6 +41,7 @@ | |||
THREAD_POOL, | |||
SHARD_STATS, | |||
MASTER_PENDING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In metricPathmap we are making map of Metric name Metric path. So, it is not possible to make same metric name for 2 different paths.
@@ -41,6 +41,7 @@ | |||
THREAD_POOL, | |||
SHARD_STATS, | |||
MASTER_PENDING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we group MASTER_PENDING
and ELECTION_TERM
into one metric MASTER_METRICS
?
@@ -0,0 +1,26 @@ | |||
/* | |||
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we change year to 2021 for all new files which need to make change to license header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
This Metric will Publish the Only Election term number
@@ -57,6 +57,8 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error for all the collectors to maintain consistency
@@ -836,14 +837,33 @@ public String toString() { | |||
|
|||
@Override | |||
public String toString() { | |||
return value; | |||
return this.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change code that you don't intend to. It changes the git blame.
This Metric will Publish the Only Election term number
Tests:
Docker Container Testing
1. Schema of Election term
2. Entry in Election term table
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.