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/03/27 05:40:06 UTC
[GitHub] [helix] zhangmeng916 opened a new pull request #917: minor fix for
customized view aggregation
zhangmeng916 opened a new pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
(#885 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This PR fixed a few of minor issues in customized aggregation view and add one more test.
### Tests
- [X] The following tests are written for this issue:
TestComputeAndCleanupCustomizedView
- [X] The following is the result of the "mvn test" command on the appropriate module:
### Commits
- [X ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [X] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399561750
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -844,14 +850,24 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
}
- for (String previousCustomizedState : lastSeenCustomizedStateTypes) {
- if (!customizedStateTypes.contains(previousCustomizedState)) {
- manager.removeListener(keyBuilder.customizedStates(instanceName, previousCustomizedState),
- this);
+ if (lastSeenCustomizedStateTypes != null) {
Review comment:
Same here, do we really need to check this?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399563146
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
##########
@@ -197,18 +197,17 @@ private void refreshTargetExternalViews(final HelixDataAccessor accessor) {
public void refreshCustomizedViewMap(final HelixDataAccessor accessor) {
// As we are not listening on customized view change, customized view will be
// refreshed once during the cache's first refresh() call, or when full refresh is required
- List<String> newStateTypes = accessor.getChildNames(accessor.keyBuilder().customizedViews());
if (_propertyDataChangedMap.get(HelixConstants.ChangeType.CUSTOMIZED_VIEW).getAndSet(false)) {
- for (String stateType : newStateTypes) {
+ for (String stateType : _aggregationEnabledTypes) {
Review comment:
This does not look right. If the config types have been changed just before the refresh with fewer items, we won't remove the extra types, right?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399619512
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,12 +830,17 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = Collections.emptySet();
+ if (lastSeenCustomizedStateTypesMap != null && lastSeenCustomizedStateTypesMap
Review comment:
Might help to simplify the code.
Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
_lastSeenCustomizedStateTypesMap.get();
if (null == lastSeenCustomizedStateTypesMap) {
lastSeenCustomizedStateTypesMap = new HashMap();
// lazy init the AtomicReference
_lastSeenCustomizedStateTypesMapRef.set(lastSeenCustomizedStateTypesMap);
}
if (!lastSeenCustomizedStateTypesMap.containsKey(instanceName)) {
lastSeenCustomizedStateTypesMap.put(instanceName, Collections.emptySet());
}
Set<String> lastSeenCustomizedStateTypes = lastSeenCustomizedStateTypesMap.get(instanceName);
Then you don't need to check null later.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399619039
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -139,7 +139,9 @@
final AtomicReference<Map<String, LiveInstance>> _lastSeenInstances;
final AtomicReference<Map<String, LiveInstance>> _lastSeenSessions;
- final AtomicReference<Set<String>> _lastSeenCustomizedStateTypes;
+ // map that stores the mapping between instance and the customized state types available on that
+ //instance
+ final AtomicReference<Map<String, Set<String>>> _lastSeenCustomizedStateTypesMap;
Review comment:
nit, since I am confused again while reviewing the code related to this field, shall we rename it to _lastSeenCustomizedStateTypesRef Or _lastSeenCustomizedStateTypesMapRef?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399698129
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,12 +830,17 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = Collections.emptySet();
+ if (lastSeenCustomizedStateTypesMap != null && lastSeenCustomizedStateTypesMap
+ .containsKey(instanceName)) {
+ lastSeenCustomizedStateTypes = lastSeenCustomizedStateTypesMap.get(instanceName);
+ }
for (String customizedState : customizedStateTypes) {
try {
- if (lastSeenCustomizedStateTypes == null || !lastSeenCustomizedStateTypes
- .contains(customizedState)) {
+ if (!lastSeenCustomizedStateTypes.contains(customizedState)) {
manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
logger.info(
Review comment:
I think it can still provide some information, so keep it for now.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #917: minor fix for
customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399561072
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -139,7 +139,7 @@
final AtomicReference<Map<String, LiveInstance>> _lastSeenInstances;
final AtomicReference<Map<String, LiveInstance>> _lastSeenSessions;
- final AtomicReference<Set<String>> _lastSeenCustomizedStateTypes;
+ final AtomicReference<Map<String, Set<String>>> _lastSeenCustomizedStateTypesMap;
Review comment:
More comments about the content of the map, please.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399561268
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,8 +828,14 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = new HashSet<>();
Review comment:
nit Collections.emtpySet()
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399625906
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,12 +830,17 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = Collections.emptySet();
+ if (lastSeenCustomizedStateTypesMap != null && lastSeenCustomizedStateTypesMap
+ .containsKey(instanceName)) {
+ lastSeenCustomizedStateTypes = lastSeenCustomizedStateTypesMap.get(instanceName);
+ }
for (String customizedState : customizedStateTypes) {
try {
- if (lastSeenCustomizedStateTypes == null || !lastSeenCustomizedStateTypes
- .contains(customizedState)) {
+ if (!lastSeenCustomizedStateTypes.contains(customizedState)) {
manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
logger.info(
Review comment:
In ZkHelixManager, there's log for addListener, but in the controller, all the operations of adding listeners have this log, so I think it's better to keep consistent.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399629387
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -851,7 +865,7 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
}
- _lastSeenCustomizedStateTypes.set(new HashSet<>(customizedStateTypes));
+ lastSeenCustomizedStateTypes = new HashSet<>(customizedStateTypes);
Review comment:
Are you trying to do the following logic?
// Update the last seen types cache
lastSeenCustomizedStateTypes.clear()
lastSeenCustomizedStateTypes.addAll(customizedStateTypes)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399563673
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
##########
@@ -197,18 +197,17 @@ private void refreshTargetExternalViews(final HelixDataAccessor accessor) {
public void refreshCustomizedViewMap(final HelixDataAccessor accessor) {
// As we are not listening on customized view change, customized view will be
// refreshed once during the cache's first refresh() call, or when full refresh is required
- List<String> newStateTypes = accessor.getChildNames(accessor.keyBuilder().customizedViews());
if (_propertyDataChangedMap.get(HelixConstants.ChangeType.CUSTOMIZED_VIEW).getAndSet(false)) {
- for (String stateType : newStateTypes) {
+ for (String stateType : _aggregationEnabledTypes) {
if (!_customizedViewCacheMap.containsKey(stateType)) {
CustomizedViewCache newCustomizedViewCache =
new CustomizedViewCache(getClusterName(), stateType);
_customizedViewCacheMap.put(stateType, newCustomizedViewCache);
}
_customizedViewCacheMap.get(stateType).refresh(accessor);
}
- Set<String> previousCachedStateTypes = _customizedViewCacheMap.keySet();
- previousCachedStateTypes.removeAll(newStateTypes);
+ Set<String> previousCachedStateTypes = new HashSet<>(_customizedViewCacheMap.keySet());
+ previousCachedStateTypes.removeAll(_aggregationEnabledTypes);
logger.info("Remove customizedView for state: " + previousCachedStateTypes);
removeCustomizedViewTypes(new ArrayList<>(previousCachedStateTypes));
Review comment:
The previous change is a good catch. But I don't think we need this converting if we just make removeCustomizedViewTypes receives a set.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399618916
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -844,14 +850,24 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
}
- for (String previousCustomizedState : lastSeenCustomizedStateTypes) {
- if (!customizedStateTypes.contains(previousCustomizedState)) {
- manager.removeListener(keyBuilder.customizedStates(instanceName, previousCustomizedState),
- this);
+ if (lastSeenCustomizedStateTypes != null) {
Review comment:
Did you mean to remove this check or not?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399618804
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
##########
@@ -197,18 +197,17 @@ private void refreshTargetExternalViews(final HelixDataAccessor accessor) {
public void refreshCustomizedViewMap(final HelixDataAccessor accessor) {
// As we are not listening on customized view change, customized view will be
// refreshed once during the cache's first refresh() call, or when full refresh is required
- List<String> newStateTypes = accessor.getChildNames(accessor.keyBuilder().customizedViews());
if (_propertyDataChangedMap.get(HelixConstants.ChangeType.CUSTOMIZED_VIEW).getAndSet(false)) {
- for (String stateType : newStateTypes) {
+ for (String stateType : _aggregationEnabledTypes) {
Review comment:
I think I misread. This is for the in-memory cache not for the znodes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399570055
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
##########
@@ -197,18 +197,17 @@ private void refreshTargetExternalViews(final HelixDataAccessor accessor) {
public void refreshCustomizedViewMap(final HelixDataAccessor accessor) {
// As we are not listening on customized view change, customized view will be
// refreshed once during the cache's first refresh() call, or when full refresh is required
- List<String> newStateTypes = accessor.getChildNames(accessor.keyBuilder().customizedViews());
if (_propertyDataChangedMap.get(HelixConstants.ChangeType.CUSTOMIZED_VIEW).getAndSet(false)) {
- for (String stateType : newStateTypes) {
+ for (String stateType : _aggregationEnabledTypes) {
Review comment:
In refresh logic, the refresh of customized state config (aggregationEnableTypes) happened before customized view. If we read stale aggregationEnabledTypes, it means that the event of customized state config change has not been processed yet. And it should be fine we do not remove stale types, right?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399629132
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,12 +830,17 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = Collections.emptySet();
+ if (lastSeenCustomizedStateTypesMap != null && lastSeenCustomizedStateTypesMap
+ .containsKey(instanceName)) {
+ lastSeenCustomizedStateTypes = lastSeenCustomizedStateTypesMap.get(instanceName);
+ }
for (String customizedState : customizedStateTypes) {
try {
- if (lastSeenCustomizedStateTypes == null || !lastSeenCustomizedStateTypes
- .contains(customizedState)) {
+ if (!lastSeenCustomizedStateTypes.contains(customizedState)) {
manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
logger.info(
Review comment:
The only additional information we get by printing this log is manager.getInstanceName(), I guess. I don't have a strong preference. But do whatever the other similar code does might cause redundant code. Please only keep this log if you think the extra helixmanager name can help us to debug in the future.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399564269
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregationStage.java
##########
@@ -119,6 +117,7 @@ public void execute(final ClusterEvent event) throws Exception {
"Failed to calculate customized view for resource " + resource.getResourceName(), ex);
}
}
+ updatedCustomizedViews.clear();
Review comment:
Why? This list is a local variable, right?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399571323
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/CustomizedViewAggregationStage.java
##########
@@ -119,6 +117,7 @@ public void execute(final ClusterEvent event) throws Exception {
"Failed to calculate customized view for resource " + resource.getResourceName(), ex);
}
}
+ updatedCustomizedViews.clear();
Review comment:
It's a variable defined outside state type for loop. If we do not clear it, it actually contains resource -> customized view mapping for all state types. When we update ZK, there's problem as we are updating state by state.
But I actually found this change is ugly, so I just moved the list inside the loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399561676
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,8 +828,14 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = new HashSet<>();
+ if (lastSeenCustomizedStateTypesMap != null && lastSeenCustomizedStateTypesMap
+ .containsKey(instanceName)) {
+ lastSeenCustomizedStateTypes = lastSeenCustomizedStateTypesMap.get(instanceName);
+ }
for (String customizedState : customizedStateTypes) {
try {
if (lastSeenCustomizedStateTypes == null || !lastSeenCustomizedStateTypes
Review comment:
lastSeenCustomizedStateTypes won't be null, right?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on issue #917: minor fix for
customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on issue #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#issuecomment-606188470
This PR is read to be merged, approved by @jiajunwang
Final commit message:
Fix minor issues in customized view aggregation logic and add some more tests.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399632030
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -851,7 +865,7 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
}
- _lastSeenCustomizedStateTypes.set(new HashSet<>(customizedStateTypes));
+ lastSeenCustomizedStateTypes = new HashSet<>(customizedStateTypes);
Review comment:
Yeah, messed it up.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #917:
minor fix for customized view aggregation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #917: minor fix for customized view aggregation
URL: https://github.com/apache/helix/pull/917#discussion_r399619213
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -828,12 +830,17 @@ public void onCustomizedStateRootChange(String instanceName, List<String> custom
}
// TODO: remove the synchronization here once we move this update into dataCache.
- synchronized (_lastSeenCustomizedStateTypes) {
- Set<String> lastSeenCustomizedStateTypes = _lastSeenCustomizedStateTypes.get();
+ synchronized (_lastSeenCustomizedStateTypesMap) {
+ Map<String, Set<String>> lastSeenCustomizedStateTypesMap =
+ _lastSeenCustomizedStateTypesMap.get();
+ Set<String> lastSeenCustomizedStateTypes = Collections.emptySet();
+ if (lastSeenCustomizedStateTypesMap != null && lastSeenCustomizedStateTypesMap
+ .containsKey(instanceName)) {
+ lastSeenCustomizedStateTypes = lastSeenCustomizedStateTypesMap.get(instanceName);
+ }
for (String customizedState : customizedStateTypes) {
try {
- if (lastSeenCustomizedStateTypes == null || !lastSeenCustomizedStateTypes
- .contains(customizedState)) {
+ if (!lastSeenCustomizedStateTypes.contains(customizedState)) {
manager.addCustomizedStateChangeListener(this, instanceName, customizedState);
logger.info(
Review comment:
nit, if addCustomizedStateChangeListener already logs inside, we don't need this info log. Please confirm.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org