Skip to content

HDDS-5536. Add metrics for ReplicationSupervisor#2493

Merged
ChenSammi merged 3 commits intoapache:masterfrom
ChenSammi:HDDS-5536
Oct 8, 2021
Merged

HDDS-5536. Add metrics for ReplicationSupervisor#2493
ChenSammi merged 3 commits intoapache:masterfrom
ChenSammi:HDDS-5536

Conversation

@ChenSammi
Copy link
Contributor

@adoroszlai adoroszlai changed the title Add metrics for ReplicationSupervisor HDDS-5536. Add metrics for ReplicationSupervisor Aug 4, 2021
public static ReplicationSupervisorMetrics create(ReplicationSupervisor
supervisor) {
MetricsSystem ms = DefaultMetricsSystem.instance();
return ms.register(SOURCE, "Container Replication Supervisor Metrics",
Copy link
Contributor

Choose a reason for hiding this comment

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

we create and register one MetricsSystem per DN, which is not a problem for typical deployments.
But i imagine it can potentially be problematic for unit tests because there are multiple DN objects in unit test heap and only one of them will successfully register.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other than that the code looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, that will be a "metrics already exists" warning in unit test, not only for this metrics, but also for other DN metries.
What's a general way to handle this? Maybe we can append DN's hostname+port to each metrics source name?
I'm not sure how HDFS handle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

HDFS DataNodeMetrics.java
dnName is host name + port. So yeah, it would be a good idea to try to make them unique.
public static DataNodeMetrics create(Configuration conf, String dnName) {
String sessionId = conf.get(DFSConfigKeys.DFS_METRICS_SESSION_ID_KEY);
MetricsSystem ms = DefaultMetricsSystem.instance();
JvmMetrics jm = JvmMetrics.create("DataNode", sessionId, ms);
String name = "DataNodeActivity-"+ (dnName.isEmpty()
? "UndefinedDataNodeName"+ ThreadLocalRandom.current().nextInt()
: dnName.replace(':', '-'));

// Percentile measurement is off by default, by watching no intervals
int[] intervals = 
    conf.getInts(DFSConfigKeys.DFS_METRICS_PERCENTILES_INTERVALS_KEY);

return ms.register(name, null, new DataNodeMetrics(name, sessionId,
    intervals, jm));

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@ChenSammi ChenSammi Aug 13, 2021

Choose a reason for hiding this comment

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

I see. @jojochuang , can we do it in a followup JIRA? since other DN metrics also need be improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jojochuang can we commit this and file a follow-up task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fired HDDS-5838 to track the comments.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ChenSammi for the patch.

@ChenSammi ChenSammi merged commit 57729b4 into apache:master Oct 8, 2021
@ChenSammi
Copy link
Contributor Author

Thanks @jojochuang and @adoroszlai for the code review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants