jiajunwang commented on a change in pull request #1111:
URL: https://github.com/apache/helix/pull/1111#discussion_r447361253
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterStatusMonitor.java
##########
@@ -289,6 +288,32 @@ public void setClusterInstanceStatus(Set<String>
liveInstanceSet, Set<String> in
}
}
}
+
+ // update all message related gauges
Review comment:
nit, can we organically add the new logic to the existing one. As you
can see the logic is,
1. ensure instance mbeans created
2. do the metric data calculation
3. update the metrics
Can we split the new logic into 2 parts and put the logic to the
corresponding sections 2 and 3?
1. We have some check regarding the monitor objects there.
2. We want to keep the structure of the code.
##########
File path:
helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterStatusMonitor.java
##########
@@ -881,7 +910,7 @@ public String clusterBeanName() {
* @param instanceName
* @return instance bean name
*/
- private String getInstanceBeanName(String instanceName) {
+ public String getInstanceBeanName(String instanceName) {
Review comment:
I don't have strong preference here.
- Shall protected or package-private enough here?
- Or, on the other hand, with these two methods made to the public, we can
avoid several test codes using INSTANCE_DN_KEY and RESOURCE_DN_KEY directly.
Not a must in this PR, though.
##########
File path: helix-core/src/main/java/org/apache/helix/model/Message.java
##########
@@ -522,12 +538,25 @@ public long getExecuteStartTimeStamp() {
/**
* Get the time that this message was created
- * @return UNIX timestamp
+ * @return UNIX epoch timestamp
*/
public long getCreateTimeStamp() {
return _record.getLongField(Attributes.CREATE_TIMESTAMP.toString(), 0L);
}
+ /**
+ * Get the time that the message was expected to be completed
+ * @return UNIX epoch timestamp
+ */
+ public long getCompletionDueTimeStamp() {
+ long completionDue =
_record.getLongField(Attributes.COMPLETION_DUE_TIMESTAMP.name(), 0L);
+ if (completionDue == 0) {
+ completionDue = getCreateTimeStamp() + MESSAGE_EXPECT_COMPLETION_PERIOD;
+ }
Review comment:
To be safe, shall we define a constant COMPLETION_DUE_TIMESTAMP_NOT_SET
= -1 for the default value?
##########
File path: helix-core/src/main/java/org/apache/helix/model/Message.java
##########
@@ -173,6 +181,14 @@ public void setCreateTimeStamp(long timestamp) {
_record.setLongField(Attributes.CREATE_TIMESTAMP.toString(), timestamp);
}
+ /**
+ * Set the time that the message was expected to be completed
+ * @param timestamp a UNIX epoch timestamp
+ */
+ public void setCompletionDueTimeStamp(long timestamp) {
+ _record.setLongField(Attributes.COMPLETION_DUE_TIMESTAMP.name(),
timestamp);
Review comment:
Is this method set by the controller or the participant?
If controller, can we just update it when set the create time? So we don't
need to consider validating.
Or, if the messages are created with different expected run time, then we
can add the expected run time as the parameter. That logic would easier to
justify (and easier to valid) compared with passing the exact due timestamp.
And I am thinking that it might be better if the participant updates it.
Since 1. participant knows the estimation better. 2. if the participant has not
read the message yet, then we probably don't want to mark it as a past-due
message.
----------------------------------------------------------------
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]
