jiajunwang commented on a change in pull request #1732:
URL: https://github.com/apache/helix/pull/1732#discussion_r630585446
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterStatusMonitor.java
##########
@@ -898,26 +896,19 @@ private void unregisterPerInstanceResources(
throws MalformedObjectNameException {
synchronized (_perInstanceResourceMonitorMap) {
for (PerInstanceResourceMonitor.BeanName beanName : beanNames) {
- unregister(getObjectName(
- getPerInstanceResourceBeanName(beanName.instanceName(),
beanName.resourceName())));
+ _perInstanceResourceMonitorMap.get(beanName).unregister();
Review comment:
Better to check null to avoid NPE.
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterStatusMonitor.java
##########
@@ -710,8 +709,8 @@ private String preProcessWorkflow(String workflowType) {
if (!_perTypeWorkflowMonitorMap.containsKey(workflowType)) {
WorkflowMonitor monitor = new WorkflowMonitor(_clusterName,
workflowType);
try {
- registerWorkflow(monitor);
- } catch (MalformedObjectNameException e) {
+ monitor.register();
+ } catch (Exception e) {
Review comment:
Should only throw JMException? And the same question to the other
catches.
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/PerInstanceResourceMonitor.java
##########
@@ -113,6 +118,10 @@ public String getResourceName() {
return _resourceName;
}
+ public String getBeanName() {
Review comment:
Is this method used anywhere? And the return string seems not to be a
valid MBean name, which should look like, "%s,%s=%s,%s=%s"
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/WorkflowMonitor.java
##########
@@ -142,12 +113,72 @@ public void resetGauges() {
*/
public void updateWorkflowGauges(TaskState current) {
if (current == null || current.equals(TaskState.NOT_STARTED)) {
- _queuedWorkflowGauge++;
+ incrementSimpleDynamicMetric(_queuedWorkflowGauge, 1);
} else if (current.equals(TaskState.IN_PROGRESS)) {
- _runningWorkflowGauge++;
+ incrementSimpleDynamicMetric(_runningWorkflowGauge, 1);
} else if (current.equals(TaskState.FAILED)) {
- _failedWorkflowGauge++;
+ incrementSimpleDynamicMetric(_failedWorkflowGauge, 1);
}
- _existingWorkflowGauge++;
+ incrementSimpleDynamicMetric(_existingWorkflowGauge);
+ }
+
+ // All the get functions are for testing purpose only.
+ public long getSuccessfulWorkflowCount() {
+ return _successfulWorkflowCount.getValue();
+ }
+
+ public long getFailedWorkflowCount() {
+ return _failedWorkflowCount.getValue();
+ }
+
+ public long getFailedWorkflowGauge() {
+ return _failedWorkflowGauge.getValue();
+ }
+
+ public long getExistingWorkflowGauge() {
+ return _existingWorkflowGauge.getValue();
+ }
+
+ public long getQueuedWorkflowGauge() {
+ return _queuedWorkflowGauge.getValue();
+ }
+
+ public long getRunningWorkflowGauge() {
+ return _runningWorkflowGauge.getValue();
+ }
+
+ @Override
+ public DynamicMBeanProvider register() throws JMException {
+ List<DynamicMetric<?, ?>> attributeList = new ArrayList<>();
+ attributeList.add(_successfulWorkflowCount);
+ attributeList.add(_failedWorkflowCount);
+ attributeList.add(_failedWorkflowGauge);
+ attributeList.add(_existingWorkflowGauge);
+ attributeList.add(_queuedWorkflowGauge);
+ attributeList.add(_runningWorkflowGauge);
+ attributeList.add(_totalWorkflowLatencyCount);
+ attributeList.add(_maximumWorkflowLatencyGauge);
+ attributeList.add(_lastResetTime);
+ String workflowBeanName = getWorkflowBeanName(_workflowType);
+ doRegister(attributeList, MBEAN_DESCRIPTION,
getObjectName(workflowBeanName));
+ return this;
+ }
+
+ private ObjectName getObjectName(String name) throws
MalformedObjectNameException {
+ return new ObjectName(String.format("%s:%s",
MonitorDomainNames.ClusterStatus.name(), name));
+ }
+
+ /**
+ * Build workflow per type bean name
+ * "cluster={clusterName},workflowType={workflowType},
+ * @param workflowType The workflow type
+ * @return per workflow type bean name
+ */
+ private String getWorkflowBeanName(String workflowType) {
+ return String.format("%s, %s=%s", clusterBeanName(), WORKFLOW_TYPE_DN_KEY,
workflowType);
+ }
+
+ private String clusterBeanName() {
+ return String.format("%s=%s", CLUSTER_DN_KEY, _clusterName);
}
Review comment:
I think these nested String.format calls are too verbose. If they are
not shared, can we just make it one single method?
##########
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:
These are just for generating the Object name, since BeanName won't be
used in the ClusterStatusMonitor anymore, can we just make it private and
change the logic to return a complete ObjectName?
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/WorkflowMonitor.java
##########
@@ -142,12 +113,72 @@ public void resetGauges() {
*/
public void updateWorkflowGauges(TaskState current) {
if (current == null || current.equals(TaskState.NOT_STARTED)) {
- _queuedWorkflowGauge++;
+ incrementSimpleDynamicMetric(_queuedWorkflowGauge, 1);
Review comment:
Not for this PR, but this method seems to be a better fit in the
SimpleDynamicMetric class
--
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]
