You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "zpinto (via GitHub)" <gi...@apache.org> on 2023/11/21 00:54:44 UTC
[PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
zpinto opened a new pull request, #2702:
URL: https://github.com/apache/helix/pull/2702
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- 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
- 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2702:
URL: https://github.com/apache/helix/pull/2702#issuecomment-1823437231
Failed tests on previous execution failed due to known flaky tests:
Test failed: testGetAllInstances(org.apache.helix.rest.server.TestInstancesAccessor) https://github.com/apache/helix/issues/2667 (there is PR for fix on this one)
testCacheDataUpdates(org.apache.helix.metaclient.impl.zk.TestZkMetaClientCache) https://github.com/apache/helix/issues/2693
I was able to pass both of these tests locally.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2702:
URL: https://github.com/apache/helix/pull/2702#issuecomment-1830318390
Thank you so much for the review @xyuanlu!
This PR is ready to be merged.
Final Commit Message:
Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2702:
URL: https://github.com/apache/helix/pull/2702#issuecomment-1823581916
Had an offline review session and discussed several points. Generally looking good. Thanks for working on 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2702:
URL: https://github.com/apache/helix/pull/2702#issuecomment-1823581391
Will add comment with TODO to address 2->3 ST messages not being sent and throttled due to more messages being sent to SWAP_IN and counting against throttles.
This is also a known issue already and we should figure out a way to prioritize ST messages.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2702:
URL: https://github.com/apache/helix/pull/2702#discussion_r1402461649
##########
helix-core/src/main/java/org/apache/helix/util/HelixUtil.java:
##########
@@ -295,7 +295,7 @@ private static Map<String, ResourceAssignment> getAssignmentForWagedFullAutoImpl
// For LiveInstances, we must preserve the existing session IDs
// So read LiveInstance objects from the cluster and do a "retainAll" on them
// liveInstanceMap is an unmodifiableMap instances, so we filter using a stream
- Map<String, LiveInstance> liveInstanceMap = dataProvider.getLiveInstances();
+ Map<String, LiveInstance> liveInstanceMap = dataProvider.getAssignableLiveInstances();
Review Comment:
nit: change name?
##########
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java:
##########
@@ -239,7 +290,8 @@ private boolean validateOfflineInstancesLimit(final ResourceControllerDataProvid
final HelixManager manager) {
int maxOfflineInstancesAllowed = cache.getClusterConfig().getMaxOfflineInstancesAllowed();
if (maxOfflineInstancesAllowed >= 0) {
- int offlineCount = cache.getAllInstances().size() - cache.getEnabledLiveInstances().size();
+ int offlineCount =
+ cache.getAssignableInstances().size() - cache.getAssignableEnabledLiveInstances().size();
Review Comment:
Here might be tricky for Evacuate. For swap, because swap-in instances are mirror, so it is not considered in offline limit for EMM. But Evacuate will reduce capacity, we may want to separate these 2 cases.
Not related to this change though.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2702:
URL: https://github.com/apache/helix/pull/2702#discussion_r1402752730
##########
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java:
##########
@@ -121,6 +131,47 @@ public void process(ClusterEvent event) throws Exception {
});
}
+ private void addSwapInInstancesToBestPossibleState(Map<String, Resource> resourceMap,
+ BestPossibleStateOutput bestPossibleStateOutput, ResourceControllerDataProvider cache) {
+ // 1. Get all SWAP_OUT instances and corresponding SWAP_IN instance pairs in the cluster.
+ Map<String, String> swapOutToSwapInInstancePairs = cache.getSwapOutToSwapInInstancePairs();
+ // 2. Get all enabled and live SWAP_IN instances in the cluster.
+ Set<String> enabledLiveSwapInInstances = cache.getEnabledLiveSwapInInstanceNames();
+ // 3. For each SWAP_OUT instance in any of the preferenceLists, add the corresponding SWAP_IN instance to the end.
+ // Skipping this when there are not SWAP_IN instances ready(enabled and live) will reduce computation time when there is not an active
+ // swap occurring.
+ if (!enabledLiveSwapInInstances.isEmpty() && !cache.isMaintenanceModeEnabled()) {
+ resourceMap.forEach((resourceName, resource) -> {
+ StateModelDefinition stateModelDef = cache.getStateModelDef(resource.getStateModelDefRef());
+ bestPossibleStateOutput.getResourceStatesMap().get(resourceName).getStateMap()
+ .forEach((partition, stateMap) -> {
+ Set<String> commonInstances = new HashSet<>(stateMap.keySet());
+ commonInstances.retainAll(swapOutToSwapInInstancePairs.keySet());
+
+ if (commonInstances.isEmpty()) {
+ return;
+ }
+
+ for (String swapOutInstance : commonInstances) {
+ String swapInInstanceState;
+ // If the swap-out instance's replica is a topState and the StateModel allows for
+ // another replica with the topState to be added, set the swap-in instance's replica
+ // to the topState. Otherwise, set the swap-in instance's replica to the secondTopState.
+ if (stateMap.get(swapOutInstance).equals(stateModelDef.getTopState()) &&
+ AbstractRebalancer.getStateCount(stateModelDef.getTopState(), stateModelDef,
+ stateMap.size() + 1, stateMap.size() + 1) > stateMap.size()) {
+ swapInInstanceState = stateModelDef.getTopState();
+ } else {
+ swapInInstanceState = stateModelDef.getSecondTopStates().iterator().next();
Review Comment:
We are assuming swap out instance has 2nd top state here. Double check on 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2702:
URL: https://github.com/apache/helix/pull/2702#discussion_r1399994013
##########
helix-core/src/main/java/org/apache/helix/model/ResourceAssignment.java:
##########
@@ -23,6 +23,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
+import java.util.stream.Collectors;
Review Comment:
Used anywhere?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2702:
URL: https://github.com/apache/helix/pull/2702#issuecomment-1829363377
Was able to fix TestInstanceAccessor.getAllInstances test.
Test failed: testCacheDataUpdates(org.apache.helix.metaclient.impl.zk.TestZkMetaClientCache) https://github.com/apache/helix/issues/2693 which is now fixed by https://github.com/apache/helix/pull/2705
Assuming that review looks good, this PR is ready to be merged.
Final Commit Message:
Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2702:
URL: https://github.com/apache/helix/pull/2702#discussion_r1402752730
##########
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java:
##########
@@ -121,6 +131,47 @@ public void process(ClusterEvent event) throws Exception {
});
}
+ private void addSwapInInstancesToBestPossibleState(Map<String, Resource> resourceMap,
+ BestPossibleStateOutput bestPossibleStateOutput, ResourceControllerDataProvider cache) {
+ // 1. Get all SWAP_OUT instances and corresponding SWAP_IN instance pairs in the cluster.
+ Map<String, String> swapOutToSwapInInstancePairs = cache.getSwapOutToSwapInInstancePairs();
+ // 2. Get all enabled and live SWAP_IN instances in the cluster.
+ Set<String> enabledLiveSwapInInstances = cache.getEnabledLiveSwapInInstanceNames();
+ // 3. For each SWAP_OUT instance in any of the preferenceLists, add the corresponding SWAP_IN instance to the end.
+ // Skipping this when there are not SWAP_IN instances ready(enabled and live) will reduce computation time when there is not an active
+ // swap occurring.
+ if (!enabledLiveSwapInInstances.isEmpty() && !cache.isMaintenanceModeEnabled()) {
+ resourceMap.forEach((resourceName, resource) -> {
+ StateModelDefinition stateModelDef = cache.getStateModelDef(resource.getStateModelDefRef());
+ bestPossibleStateOutput.getResourceStatesMap().get(resourceName).getStateMap()
+ .forEach((partition, stateMap) -> {
+ Set<String> commonInstances = new HashSet<>(stateMap.keySet());
+ commonInstances.retainAll(swapOutToSwapInInstancePairs.keySet());
+
+ if (commonInstances.isEmpty()) {
+ return;
+ }
+
+ for (String swapOutInstance : commonInstances) {
+ String swapInInstanceState;
+ // If the swap-out instance's replica is a topState and the StateModel allows for
+ // another replica with the topState to be added, set the swap-in instance's replica
+ // to the topState. Otherwise, set the swap-in instance's replica to the secondTopState.
+ if (stateMap.get(swapOutInstance).equals(stateModelDef.getTopState()) &&
+ AbstractRebalancer.getStateCount(stateModelDef.getTopState(), stateModelDef,
+ stateMap.size() + 1, stateMap.size() + 1) > stateMap.size()) {
+ swapInInstanceState = stateModelDef.getTopState();
+ } else {
+ swapInInstanceState = stateModelDef.getSecondTopStates().iterator().next();
Review Comment:
We are assuming swap out instance has 2nd top state here. Doule check on think.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2702:
URL: https://github.com/apache/helix/pull/2702#discussion_r1400037249
##########
helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java:
##########
@@ -335,6 +338,107 @@ private void refreshInstanceConfigs(final HelixDataAccessor accessor,
}
}
+ /**
+ * Refreshes the assignable instances and SWAP related caches. This should be called after
+ * liveInstance and instanceConfig caches are refreshed. To determine what instances are
+ * assignable and live, it takes a combination of both the all instanceConfigs and liveInstances.
+ *
+ * @param instanceConfigMap InstanceConfig map from instanceConfig cache
+ * @param liveInstancesMap LiveInstance map from liveInstance cache
+ * @param clusterConfig ClusterConfig from clusterConfig cache
+ */
Review Comment:
Please add TODO for evacuation as well.
##########
helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java:
##########
@@ -103,7 +103,9 @@ public class BaseControllerDataProvider implements ControlContextProvider {
// Property caches
private final PropertyCache<ResourceConfig> _resourceConfigCache;
private final PropertyCache<InstanceConfig> _instanceConfigCache;
+ private final Map<String, InstanceConfig> _assignableInstanceConfigMap = new HashMap<>();
Review Comment:
nit: Probably we can move these 2 lines out of the `Property caches` code block and have more detailed comments. Like what instances are counted as "assignable".
##########
helix-core/src/main/java/org/apache/helix/controller/changedetector/ResourceChangeSnapshot.java:
##########
@@ -82,10 +82,10 @@ class ResourceChangeSnapshot {
_changedTypes = new HashSet<>(dataProvider.getRefreshedChangeTypes());
_instanceConfigMap = ignoreNonTopologyChange ?
Review Comment:
Do we want to change the name of `_instanceConfigMap ` as well? If user call `getInstanceConfigMap` it may be confusing.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2702:
URL: https://github.com/apache/helix/pull/2702
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2702:
URL: https://github.com/apache/helix/pull/2702#discussion_r1402589370
##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/DelayedAutoRebalancer.java:
##########
@@ -199,24 +194,11 @@ public IdealState computeNewIdealState(String resourceName,
finalMapping.getListFields().putAll(userDefinedPreferenceList);
- // 1. Get all SWAP_OUT instances and corresponding SWAP_IN instance pairs in the cluster.
- Map<String, String> swapOutToSwapInInstancePairs =
- clusterData.getSwapOutToSwapInInstancePairs();
- // 2. Get all enabled and live SWAP_IN instances in the cluster.
- Set<String> enabledLiveSwapInInstances = clusterData.getEnabledLiveSwapInInstanceNames();
- // 3. For each SWAP_OUT instance in any of the preferenceLists, add the corresponding SWAP_IN instance to the end.
- // Skipping this when there are not SWAP_IN instances ready(enabled and live) will reduce computation time when there is not an active
- // swap occurring.
- if (!clusterData.getEnabledLiveSwapInInstanceNames().isEmpty()) {
- DelayedRebalanceUtil.addSwapInInstanceToPreferenceListsIfSwapOutInstanceExists(finalMapping,
- swapOutToSwapInInstancePairs, enabledLiveSwapInInstances);
- }
-
LOG.debug("currentMapping: {}", currentMapping);
LOG.debug("stateCountMap: {}", stateCountMap);
LOG.debug("liveEnabledNodes: {}", liveEnabledNodes);
Review Comment:
Change name in log?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Fix WAGED to only use logicalId when computing baseline and centralize picking assignable instances in the cache [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2702:
URL: https://github.com/apache/helix/pull/2702#issuecomment-1823571222
> Thanks for making the change, One general question: for spectator and EV, I think there are places where we still use `getInstanceConfigMap`. Will this include both swap in/out instances?
Will be addressing this in a future PR for spectator.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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