jiajunwang commented on a change in pull request #1187:
URL: https://github.com/apache/helix/pull/1187#discussion_r465397897
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedStateComputationStage.java
##########
@@ -86,6 +86,9 @@ private void updateCustomizedStates(String instanceName,
String customizedStateT
customizedStateOutput
.setCustomizedState(customizedStateType, resourceName,
partition, instanceName,
customizedState.getState(partitionName));
+ customizedStateOutput
+ .setStartTime(customizedStateType, resourceName, partition,
instanceName,
+ String.valueOf(customizedState.getStartTime(partitionName)));
Review comment:
Can you process the long type inside the method? Converting the type in
the caller is not clean. If you have multiple callers, then you will need to
convert every time.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedStateComputationStage.java
##########
@@ -86,6 +86,9 @@ private void updateCustomizedStates(String instanceName,
String customizedStateT
customizedStateOutput
.setCustomizedState(customizedStateType, resourceName,
partition, instanceName,
customizedState.getState(partitionName));
+ customizedStateOutput
Review comment:
Will it be simpler if you set the start time inside
setCustomizedState()? Are there any other cases that you want to update the
state but don't touch the start time?
This will help to reduce duplicate code.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedStateOutput.java
##########
@@ -24,32 +24,58 @@
import java.util.Map;
import java.util.Set;
+import org.apache.helix.model.CustomizedState;
import org.apache.helix.model.Partition;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class CustomizedStateOutput {
+ private static final Logger LOG =
LoggerFactory.getLogger(CustomizedStateOutput.class);
+
// stateType -> (resourceName -> (Partition -> (instanceName ->
customizedState)))
private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_customizedStateMap;
+ // stateType -> (resourceName -> (Partition -> (instanceName -> startTime)))
+ private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_startTimeMap;
public CustomizedStateOutput() {
_customizedStateMap = new HashMap<>();
+ _startTimeMap = new HashMap<>();
}
public void setCustomizedState(String stateType, String resourceName,
Partition partition,
String instanceName, String state) {
- if (!_customizedStateMap.containsKey(stateType)) {
- _customizedStateMap
- .put(stateType, new HashMap<String, Map<Partition, Map<String,
String>>>());
- }
- if (!_customizedStateMap.get(stateType).containsKey(resourceName)) {
- _customizedStateMap.get(stateType)
- .put(resourceName, new HashMap<Partition, Map<String, String>>());
- }
- if
(!_customizedStateMap.get(stateType).get(resourceName).containsKey(partition)) {
- _customizedStateMap.get(stateType).get(resourceName)
- .put(partition, new HashMap<String, String>());
+
setCustomizedStateProperty(CustomizedState.CustomizedStateProperty.CURRENT_STATE,
stateType,
+ resourceName, partition, instanceName, state);
+ }
+
+ public void setStartTime(String stateType, String resourceName, Partition
partition,
+ String instanceName, String state) {
Review comment:
It is called set start time, but you put a "state" in the parameter. It
looks strange.
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -37,7 +37,8 @@
MonitorDomainNames.ClusterStatus.name(),
MonitorDomainNames.HelixZkClient.name(),
MonitorDomainNames.HelixCallback.name(),
- MonitorDomainNames.Rebalancer.name()
+ MonitorDomainNames.Rebalancer.name(),
+ MonitorDomainNames.CustomizedView.name()
Review comment:
CustomizedView -> AggregatedView ? In this case, we can transfer EV TEV
metrics to this domain in the future.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedStateOutput.java
##########
@@ -24,32 +24,58 @@
import java.util.Map;
import java.util.Set;
+import org.apache.helix.model.CustomizedState;
import org.apache.helix.model.Partition;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class CustomizedStateOutput {
+ private static final Logger LOG =
LoggerFactory.getLogger(CustomizedStateOutput.class);
+
// stateType -> (resourceName -> (Partition -> (instanceName ->
customizedState)))
private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_customizedStateMap;
Review comment:
I know we are doing this style (separate maps for different fields)
everywhere, but I am thinking if we can make it better. Either using the Pair
or make a private class will definitely help to simplify the code here.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregationStage.java
##########
@@ -152,6 +163,33 @@ private void computeCustomizedStateView(final Resource
resource, final String st
if (curCustomizedView == null ||
!curCustomizedView.getRecord().equals(view.getRecord())) {
// Add customized view to the list which will be written to ZK later.
updatedCustomizedViews.add(view);
+ updatedStartTimestamps.put(resourceName,
+ customizedStateOutput.getResourceStartTimeMap(stateType,
resourceName));
}
}
+
+ private CustomizedViewMonitor getOrCreateMonitor(ClusterEvent event) throws
JMException {
Review comment:
Once we put this map into the cache object, concurrent control is
necessary for this method.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregationStage.java
##########
@@ -35,17 +37,20 @@
import org.apache.helix.controller.LogUtil;
import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
import org.apache.helix.controller.pipeline.AbstractAsyncBaseStage;
+import org.apache.helix.controller.pipeline.AbstractBaseStage;
import org.apache.helix.controller.pipeline.AsyncWorkerType;
import org.apache.helix.controller.pipeline.StageException;
import org.apache.helix.model.CustomizedView;
import org.apache.helix.model.Partition;
import org.apache.helix.model.Resource;
+import org.apache.helix.monitoring.mbeans.CustomizedViewMonitor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class CustomizedViewAggregationStage extends AbstractAsyncBaseStage {
private static Logger LOG =
LoggerFactory.getLogger(CustomizedViewAggregationStage.class);
+ private Map<String, CustomizedViewMonitor> _monitors = new HashMap<>();
Review comment:
I think we shall make stage object stateless. So this monitor map shall
be in the cache.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedStateOutput.java
##########
@@ -24,32 +24,58 @@
import java.util.Map;
import java.util.Set;
+import org.apache.helix.model.CustomizedState;
import org.apache.helix.model.Partition;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class CustomizedStateOutput {
+ private static final Logger LOG =
LoggerFactory.getLogger(CustomizedStateOutput.class);
+
// stateType -> (resourceName -> (Partition -> (instanceName ->
customizedState)))
private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_customizedStateMap;
+ // stateType -> (resourceName -> (Partition -> (instanceName -> startTime)))
+ private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_startTimeMap;
public CustomizedStateOutput() {
_customizedStateMap = new HashMap<>();
+ _startTimeMap = new HashMap<>();
}
public void setCustomizedState(String stateType, String resourceName,
Partition partition,
String instanceName, String state) {
- if (!_customizedStateMap.containsKey(stateType)) {
- _customizedStateMap
- .put(stateType, new HashMap<String, Map<Partition, Map<String,
String>>>());
- }
- if (!_customizedStateMap.get(stateType).containsKey(resourceName)) {
- _customizedStateMap.get(stateType)
- .put(resourceName, new HashMap<Partition, Map<String, String>>());
- }
- if
(!_customizedStateMap.get(stateType).get(resourceName).containsKey(partition)) {
- _customizedStateMap.get(stateType).get(resourceName)
- .put(partition, new HashMap<String, String>());
+
setCustomizedStateProperty(CustomizedState.CustomizedStateProperty.CURRENT_STATE,
stateType,
+ resourceName, partition, instanceName, state);
+ }
+
+ public void setStartTime(String stateType, String resourceName, Partition
partition,
+ String instanceName, String state) {
+
setCustomizedStateProperty(CustomizedState.CustomizedStateProperty.START_TIME,
stateType,
+ resourceName, partition, instanceName, state);
+ }
+
+ private void
setCustomizedStateProperty(CustomizedState.CustomizedStateProperty propertyName,
+ String stateType, String resourceName, Partition partition, String
instanceName,
+ String state) {
+ Map<String, Map<String, Map<Partition, Map<String, String>>>> mapToUpdate;
+ switch (propertyName) {
Review comment:
These are duplicate logic with the switch block in
getCustomizedStateProperty().
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedStateOutput.java
##########
@@ -24,32 +24,58 @@
import java.util.Map;
import java.util.Set;
+import org.apache.helix.model.CustomizedState;
import org.apache.helix.model.Partition;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class CustomizedStateOutput {
+ private static final Logger LOG =
LoggerFactory.getLogger(CustomizedStateOutput.class);
+
// stateType -> (resourceName -> (Partition -> (instanceName ->
customizedState)))
private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_customizedStateMap;
+ // stateType -> (resourceName -> (Partition -> (instanceName -> startTime)))
+ private final Map<String, Map<String, Map<Partition, Map<String, String>>>>
_startTimeMap;
Review comment:
Why not save the Long type for the startTime here?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregationStage.java
##########
@@ -107,8 +114,11 @@ public void execute(final ClusterEvent event) throws
Exception {
}
// add/update customized-views from zk and cache
if (updatedCustomizedViews.size() > 0) {
- dataAccessor.setChildren(keys, updatedCustomizedViews);
+ boolean[] success = dataAccessor.setChildren(keys,
updatedCustomizedViews);
cache.updateCustomizedViews(stateType, updatedCustomizedViews);
+ asyncReportLatency(cache.getAsyncTasksThreadPool(),
getOrCreateMonitor(event),
+ new ArrayList<>(updatedCustomizedViews),
curCustomizedViewsCopy,
+ new HashMap<>(updatedStartTimestamps), success.clone(),
System.currentTimeMillis());
Review comment:
Do you really need to clone the success array?
----------------------------------------------------------------
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]
