Added tests validating fix for instances with incomplete virtual topology configuration causing partition imbalance #46
Conversation
…e that instance from selection
…-topology-default-zone
vivek8420
changed the title
vivek8420
changed the title
| * This demonstrates a bad instance received ~3x more partitions. | ||
| */ | ||
| @Test | ||
| public void testMissingVirtualConfig_whenRequiredInstanceTopologyKeysNotPresent_TopologyOverAllocationToBadInstance() { |
There was a problem hiding this comment.
too long method name, please use a shorter one. Also, don't mix camelCase and underscores.
| public class TestCrushRebalanceMissingVirtualTopology extends ZkTestBase { | ||
|
|
||
| private final String className = getShortClassName(); | ||
| private String clusterName; |
There was a problem hiding this comment.
nit : I see helix uses _ standard to define class level variable. Not a standard used in java(more of python standard), but since this is used across the repo(could be used initially to easily differentiate between class vs local) so lets continue this.
| _gSetupTool.addInstanceToCluster(clusterName, badInstanceName); | ||
|
|
||
| // Get the existing config and modify it | ||
| ConfigAccessor configAccessor = new ConfigAccessor(_gZkClient); |
There was a problem hiding this comment.
You can initialised configAccessor once in @beforeMethod, I see it being initialized multiple times.
| setupClusterWithoutRequiredInstanceTopologyKeys(); | ||
|
|
||
| // Add resource with CRUSH rebalancer strategy | ||
| _gSetupTool.addResourceToCluster(clusterName, DB_NAME, PARTITIONS, |
There was a problem hiding this comment.
Extract it to helper method to reuse across tests.
| private ClusterControllerManager controller; | ||
| private List<MockParticipantManager> participants = new ArrayList<>(); | ||
| private List<String> nodes = new ArrayList<>(); | ||
| private Set<String> allDBs = new HashSet<>(); |
There was a problem hiding this comment.
This need not be defined at class level, I see it being used locally
|
|
||
| /** | ||
| * Test the original problem: missing virtual topology configuration leads to over-allocation. | ||
| * This demonstrates a bad instance received ~3x more partitions. |
There was a problem hiding this comment.
This test will start failing once the change for not allowing the bad participant to join the cluster. I believe these tests won't be required then.
Issues
Test case verifying the fix for the Venice production incident where missing virtual topology configuration led to disproportionate partition assignment in CRUSH rebalancer.
Background: Venice HAR cluster with topology /mz_virtualZone/host/applicationInstanceId
had one instance join with missing mz_virtualZone key, causing it to receive 3.26x more partitions than expected (2,166 vs 664) due to becoming a singleton fault zone.
Description
Instances missing required fault zone configuration are now excluded from rebalancing instead of creating singleton fault zones, preventing partition imbalance.
PR for fix: https://github.com/linkedin/helix/pull/40/files
Tests
(List the names of added unit/integration tests)
testMissingVirtualConfig_whenRequiredInstanceTopologyKeysNotPresent_TopologyOverAllocationToBadInstance: Test the original problem: missing virtual topology configuration leads to over-allocation. This demonstrates a bad instance received ~2x more partitions.testMissingVirtualConfig_whenRequiredInstanceTopologyKeysIsPresent_BadInstanceExcludedFromRebalancing:Test the fixed behavior: missing virtual topology configuration leads to instance exclusion. This demonstrates the fix where bad instances are excluded from rebalancing.
(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)
(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)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)