You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/08/05 01:20:59 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1187: Add latency metric support for customized view aggregation
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org