-
Notifications
You must be signed in to change notification settings - Fork 228
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
Support zone based virtual topology assignment algorithm #2986
Support zone based virtual topology assignment algorithm #2986
Conversation
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.
We had an offline discussion.
When a new host joining the cluster, the current behavior of virtual group assignment will recompute the whole virtual topology assignment, causing the whole partition assignment shuffle.
I feel like the ideal behavior should be ore sticky. When new hosts are added, the existing hosts' mapping should not change.
helix-core/src/main/java/org/apache/helix/cloud/constants/VirtualTopologyGroupConstants.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/helix/cloud/topology/FaultZoneBasedVirtualGroupAssignmentAlgorithm.java
Show resolved
Hide resolved
helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualGroupAssignmentAlgorithm.java
Outdated
Show resolved
Hide resolved
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.
Logic looks good.
I think there are many transient in memory data structure created. We could improve the efficiency and readability. Let's talk offline.
} | ||
|
||
// Build a deep copy of the current assignment to avoid mutating it directly. | ||
Map<String, Set<String>> updatedAssignment = deepCopy(virtualZoneMapping); |
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.
we also build a deep copy in distributeUnassignedZones. I think either this or the one in distributeUnassignedZones can be skipped. (Need to rearrange the code though... for example, moving this in line 75 and remove the one in distributeUnassignedZones)
helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualGroupAssignmentAlgorithm.java
Show resolved
Hide resolved
...main/java/org/apache/helix/cloud/topology/FaultZoneBasedVirtualGroupAssignmentAlgorithm.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/helix/cloud/topology/FaultZoneBasedVirtualGroupAssignmentAlgorithm.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/helix/cloud/topology/FaultZoneBasedVirtualGroupAssignmentAlgorithm.java
Show resolved
Hide resolved
...main/java/org/apache/helix/cloud/topology/FaultZoneBasedVirtualGroupAssignmentAlgorithm.java
Show resolved
Hide resolved
...main/java/org/apache/helix/cloud/topology/FaultZoneBasedVirtualGroupAssignmentAlgorithm.java
Outdated
Show resolved
Hide resolved
Generally LGTM. nit comments. |
Thanks @xyuanlu for reviewing this PR. It's ready to merge. |
Issues
Support zone based virtual topology assignment algorithm #2985
Description
This PR support zone based virtual topology assignment algorithm. In the rest service, introduced a configurable parameter enabling users to select the desired virtual topology assignment algorithm (e.g.,instance_based or zone_based). Additionally, implemented a zone-based assignment algorithm that directly maps physical zones to virtual zones.
Tests
Changes that Break Backward Compatibility (Optional)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)