Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Added Election Term Metric #557

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Added Election Term Metric #557

wants to merge 4 commits into from

Conversation

meetshah777
Copy link
Contributor

This Metric will Publish the Only Election term number

Tests:
Docker Container Testing

1. Schema of Election term

sqlite> .schema Election_Term
CREATE TABLE Election_Term(sum double null, avg double null, min double null, max double null);

2. Entry in Election term table

sqlite> select * from Election_Term;
18.0|18.0|18.0|18.0

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This Metric will Publish the Only Election term number
@yojs yojs requested review from ktkrg and khushbr February 11, 2021 22:06
yojs
yojs previously approved these changes Feb 13, 2021
@@ -41,6 +41,7 @@
THREAD_POOL,
SHARD_STATS,
MASTER_PENDING,
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@meetshah777 meetshah777 left a 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,
Copy link
Contributor Author

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,
Copy link
Contributor

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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
yu-sun-77
yu-sun-77 previously approved these changes Mar 11, 2021
@@ -57,6 +57,8 @@

Copy link
Contributor

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;
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

4 participants