jiajunwang commented on a change in pull request #1732:
URL: https://github.com/apache/helix/pull/1732#discussion_r631465432
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/PerInstanceResourceMonitor.java
##########
@@ -131,13 +140,41 @@ public synchronized void update(Map<Partition, String>
stateMap, Set<String> tag
int cnt = 0;
for (String state : stateMap.values()) {
// Skip DROPPED and initial state (e.g. OFFLINE)
- if (state.equalsIgnoreCase(HelixDefinedState.DROPPED.name())
- || state.equalsIgnoreCase(stateModelDef.getInitialState())) {
+ if (state.equalsIgnoreCase(HelixDefinedState.DROPPED.name()) || state
+ .equalsIgnoreCase(stateModelDef.getInitialState())) {
continue;
}
cnt++;
}
- _partitions = cnt;
+ _partitions.updateValue(Long.valueOf(cnt));
+ }
+
+ @Override
+ public DynamicMBeanProvider register() throws JMException {
+ List<DynamicMetric<?, ?>> attributeList = new ArrayList<>();
+ attributeList.add(_partitions);
+ String beanName = getPerInstanceResourceBeanName(_participantName,
_resourceName);
+ doRegister(attributeList, MBEAN_DESCRIPTION, getObjectName(beanName));
+ return this;
+ }
+
+ /**
Review comment:
I don't think this comment has been addressed.
Originally, the PerInstanceResourceMonitor does not know cluster info. The
BeanName object only contains the instance and resource info. So the
registration is done in the ClusterStatusMonitor.
In this new version, since we encapsulate the register method inside
PerInstanceResourceMonitor (since now it is a DynamicMBeanProvider). Based on
this, I think there are 2 options,
1. Remove BeanName class and always let the PerInstanceResourceMonitor
initialized with the fully generated ObjectName (which includes the cluster
name). For the existing BeanName usages in the ClusterStatusMonitor, we can
change to use the ObjectName. The downside of this option is the
PerInstanceResourceMonitor needs to understand MonitorDomainNames.ClusterStatus
to generate full ObjectName.
2. Since every PerInstanceResourceMonitor is generated in the
ClusterStatusMonitor, we add a private method in the ClusterStatusMonitor to
generate full ObjectName of the PerInstanceResourceMonitor when constructing
it. And PerInstanceResourceMonitor does not need to know about the domain names
etc. For example, we are now doing this design for InstanceMonitor. In this
case, BeanName can also be removed since it is duplicated with the ObjectName
parameter.
Please consider picking up either one of these 2 designs. The current
implementation is a mix-up and the MBean name generating logic has duplicated
code. It might introduce bugs if we change in one side (BeanName), but forget
to change the other side (getObjectName).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
