Skip to content

Conversation

@YuanbenWang
Copy link
Contributor

@YuanbenWang YuanbenWang commented May 15, 2024

What changes were proposed in this pull request?

This PR aims to add percentile for ProtocolMessageMetrics.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9522

How was this patch tested?

Tested in my own cluster.

When we have config:

<property>
  <name>ozone.protocol.message.metrics.percentiles.intervals</name>
  <value>127</value>
</property>

OM:
om

SCM:
scm1
scm2
scm3

DN:
dn1

The previous review record is as follows:
#5479

@YuanbenWang
Copy link
Contributor Author

@whbing Hello~! Could you plz help review this pr?

@adoroszlai adoroszlai requested a review from kerneltime October 17, 2024 15:39
private final Map<KEY, AtomicLong> elapsedTimes =
new ConcurrentHashMap<>();

private final Map<KEY, MutableQuantiles[]> quantiles =
Copy link
Contributor

Choose a reason for hiding this comment

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

See #7392 / HDDS-11642, MutableQuantiles objects need to be stopped explicitly.

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Mostly good just two comments

Comment on lines +75 to +88
if (quantileEnable) {
MetricsRegistry registry =
new MetricsRegistry(value.toString() + "MessageMetrics");
MutableQuantiles[] mutableQuantiles =
new MutableQuantiles[intervals.length];
quantiles.put(value, mutableQuantiles);
for (int i = 0; i < intervals.length; i++) {
mutableQuantiles[i] = registry.newQuantiles(
intervals[i] + "s",
value.toString() + "rpc time in milli second",
"ops", "latencyMs", intervals[i]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please add tests to cover these new lines.

@adoroszlai
Copy link
Contributor

/pending review comments

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

review comments

@github-actions
Copy link

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

@github-actions github-actions bot closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants