Skip to content

HDDS 6060 Error getting metrics from source SCMContainerMetrics#2897

Closed
ghuangups wants to merge 1 commit intoapache:masterfrom
ghuangups:HDDS-6060
Closed

HDDS 6060 Error getting metrics from source SCMContainerMetrics#2897
ghuangups wants to merge 1 commit intoapache:masterfrom
ghuangups:HDDS-6060

Conversation

@ghuangups
Copy link
Contributor

What changes were proposed in this pull request?

Instead of just returning a wrapped set, ContainerAttribute#getCollection is returning NavigableSet.

[## What is the link to the Apache JIRA]
https://issues.apache.org/jira/browse/HDDS-6060

How was this patch tested?

(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)

@ghuangups
Copy link
Contributor Author

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

LGTM. One thing is to build the metric of state count will be expensive.
@ghuangups With the fix, have you seen the issue fixed?


if (this.attributeMap.containsKey(key)) {
return Collections.unmodifiableNavigableSet(this.attributeMap.get(key));
NavigableSet<ContainerID> returneSet = new TreeSet<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing is to build the metric of state count will be expensive.

We could avoid copy on read by making ContainerAttribute store ConcurrentSkipListSet instead of TreeSet.

if (this.attributeMap.containsKey(key)) {
return Collections.unmodifiableNavigableSet(this.attributeMap.get(key));
NavigableSet<ContainerID> returneSet = new TreeSet<>();
TreeSet<ContainerID> tmpSet = (TreeSet<ContainerID>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the cast necessary?

Suggested change
TreeSet<ContainerID> tmpSet = (TreeSet<ContainerID>)
NavigableSet<ContainerID> tmpSet =

@adoroszlai
Copy link
Contributor

@ghuangups have you had a chance to look at the comments?

@adoroszlai
Copy link
Contributor

/pending

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...)

/pending

@github-actions
Copy link

github-actions bot commented Apr 8, 2022

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 Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments