-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build Topology with only required levels (FaultZone and EndNode) #2712
Build Topology with only required levels (FaultZone and EndNode) #2712
Conversation
…l non-FaultZone or EndNode levels of the tree. This will allow for swap to work in clusters where the non-FaultZone or EndNode domain kv pairs don't directly match the swapping node.
…ate same instance config. Changes to copy getAssignableLiveInstances keySet to avoid ConcurrentModificationException.
public Topology(final List<String> allNodes, final List<String> liveNodes, | ||
final Map<String, InstanceConfig> instanceConfigMap, ClusterConfig clusterConfig) { | ||
final Map<String, InstanceConfig> instanceConfigMap, ClusterConfig clusterConfig, | ||
boolean faultZoneLevelOnly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, Please correct me if I cam wrong, I did not find anywhere we are using the default constructor. We always set faultZoneLevelOnly
to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct; however, this constructor is public and may be used outside our code base by others.
This will keep backwards compatibility, while only changing the behavior of the rebalancer to limit the backwards incompatibility concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should always keep the default constructor.
Failed CI due to flaky test testDisconnectWhenConnectionBreak #2691 This PR is ready to be merged, thanks for the review. Final commit message: |
5d6396e
into
apache:ApplicationClusterManager
Issues
Description
Tests
Changes that Break Backward Compatibility (Optional)
Changes to Topology class are not backwards compatible; however, the rebalancer will have different behavior and produce new assignment for existing clusters using more than a 2-layer topology. This constitutes bumping minor version and calling out in release notes in next open source release.
Documentation (Optional)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)