diff --git a/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java b/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java index 98076a95fd..62802159d6 100644 --- a/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java +++ b/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java @@ -64,6 +64,7 @@ import org.apache.helix.model.StateModelDefinition; import org.apache.helix.task.TaskConstants; import org.apache.helix.util.HelixUtil; +import org.apache.helix.util.InstanceUtil; import org.apache.helix.zookeeper.datamodel.ZNRecord; import org.apache.helix.zookeeper.zkclient.DataUpdater; import org.slf4j.Logger; @@ -759,16 +760,10 @@ public Set getEnabledInstances() { * auto Maintenance Mode (MAX_OFFLINE_INSTANCES_ALLOWED at entry, * NUM_OFFLINE_INSTANCES_FOR_AUTO_EXIT at exit). * - *

An instance counts when all three conditions hold: - *

- * EVACUATE and DISABLE instances are included because they cannot accept new ONLINE - * replicas; ENABLE+offline instances are included for the same reason. SWAP_IN and - * UNKNOWN are excluded because they do not represent assignable cluster capacity. + *

Membership rules live in + * {@link InstanceUtil#getInstancesUnableToAcceptOnlineReplicas(Map, java.util.Collection, long)} + * so that the controller and the read-only helix-rest endpoint that reports this number to + * clients share one implementation. * *

Used by both BestPossibleStateCalcStage (MM entry) and MaintenanceRecoveryStage * (MM exit) so that the same population is measured against each threshold. @@ -777,18 +772,8 @@ public Set getEnabledInstances() { * @return a fresh modifiable set of instance names. */ public Set getInstancesUnableToAcceptOnlineReplicas(long nowMs) { - Map instanceConfigMap = getInstanceConfigMap(); - Set result = instanceConfigMap.entrySet().stream() - .filter(e -> !InstanceConstants.UNROUTABLE_INSTANCE_OPERATIONS.contains( - e.getValue().getInstanceOperation().getOperation())) - .map(Map.Entry::getKey) - .collect(Collectors.toCollection(HashSet::new)); - result.removeAll(getEnabledLiveInstances()); - result.removeIf(name -> { - InstanceConfig cfg = instanceConfigMap.get(name); - return cfg != null && cfg.isUnderInstanceOperationMaintenance(nowMs); - }); - return result; + return InstanceUtil.getInstancesUnableToAcceptOnlineReplicas(getInstanceConfigMap(), + getLiveInstances().keySet(), nowMs); } /** diff --git a/helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java b/helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java index 0651620069..c41cbbf9d8 100644 --- a/helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java +++ b/helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java @@ -19,8 +19,12 @@ * under the License. */ +import java.util.Collection; +import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -314,6 +318,79 @@ public static void setInstanceOperation(ConfigAccessor configAccessor, } } + /** + * Returns the live instances that are also marked with + * {@link InstanceConstants.InstanceOperation#ENABLE}. These are the instances that can + * currently accept ONLINE replicas. + * + * @param instanceConfigMap all instance configs in the cluster, keyed by instance name. + * @param liveInstanceNames names of the currently live instances. + * @return a fresh modifiable set of instance names. + */ + private static Set getEnabledLiveInstances( + Map instanceConfigMap, Collection liveInstanceNames) { + if (instanceConfigMap == null || liveInstanceNames == null) { + return new HashSet<>(); + } + Set enabledLiveInstances = new HashSet<>(); + for (String instanceName : liveInstanceNames) { + InstanceConfig config = instanceConfigMap.get(instanceName); + if (config != null && config.getInstanceOperation().getOperation() + == InstanceConstants.InstanceOperation.ENABLE) { + enabledLiveInstances.add(instanceName); + } + } + return enabledLiveInstances; + } + + /** + * Returns the set of instances that count toward the cluster-wide offline budget driving + * auto Maintenance Mode ({@code MAX_OFFLINE_INSTANCES_ALLOWED} at entry, + * {@code NUM_OFFLINE_INSTANCES_FOR_AUTO_EXIT} at exit). + * + *

An instance counts when all three conditions hold: + *

+ * EVACUATE and DISABLE instances are included because they cannot accept new ONLINE + * replicas; ENABLE+offline instances are included for the same reason. SWAP_IN and + * UNKNOWN are excluded because they do not represent assignable cluster capacity. + * + *

This is the single definition of the offline-budget population. The controller + * (MM entry in {@code BestPossibleStateCalcStage}, MM exit in + * {@code MaintenanceRecoveryStage}) reaches it through + * {@code BaseControllerDataProvider#getInstancesUnableToAcceptOnlineReplicas}, and + * helix-rest exposes the same computation read-only so clients never have to reimplement + * (and drift from) these rules. + * + * @param instanceConfigMap all instance configs in the cluster, keyed by instance name. + * @param liveInstanceNames names of the currently live instances. + * @param nowMs current wall-clock millis used for marker-expiry comparison. + * @return a fresh modifiable set of instance names. + */ + public static Set getInstancesUnableToAcceptOnlineReplicas( + Map instanceConfigMap, Collection liveInstanceNames, + long nowMs) { + if (instanceConfigMap == null || instanceConfigMap.isEmpty()) { + return new HashSet<>(); + } + Set result = instanceConfigMap.entrySet().stream() + .filter(e -> e.getValue() != null) + .filter(e -> !InstanceConstants.UNROUTABLE_INSTANCE_OPERATIONS.contains( + e.getValue().getInstanceOperation().getOperation())) + .map(Map.Entry::getKey) + .collect(Collectors.toCollection(HashSet::new)); + result.removeAll(getEnabledLiveInstances(instanceConfigMap, liveInstanceNames)); + result.removeIf(name -> { + InstanceConfig config = instanceConfigMap.get(name); + return config != null && config.isUnderInstanceOperationMaintenance(nowMs); + }); + return result; + } + private static String formatMatchingInstances(List matchingInstances) { return matchingInstances.stream() .map(ic -> ic.getInstanceName() + " (operation=" diff --git a/helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestInstancesUnableToAcceptOnlineReplicas.java b/helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestInstancesUnableToAcceptOnlineReplicas.java index b8c6217d30..0f33ac7341 100644 --- a/helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestInstancesUnableToAcceptOnlineReplicas.java +++ b/helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestInstancesUnableToAcceptOnlineReplicas.java @@ -27,6 +27,7 @@ import org.apache.helix.constants.InstanceConstants; import org.apache.helix.model.InstanceConfig; +import org.apache.helix.model.LiveInstance; import org.testng.Assert; import org.testng.annotations.Test; @@ -52,7 +53,7 @@ public class TestInstancesUnableToAcceptOnlineReplicas { public void testEnableLiveExcluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.ENABLE)), - enabledLive("h1")); + liveInstances("h1")); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty(), "Healthy ENABLE+live instance must not count toward the offline budget"); } @@ -61,7 +62,7 @@ public void testEnableLiveExcluded() { public void testEnableOfflineIncluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.ENABLE)), - enabledLive()); + liveInstances()); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1"), "ENABLE+offline is the canonical 'real outage' case and must count"); } @@ -70,7 +71,7 @@ public void testEnableOfflineIncluded() { public void testDisableLiveIncluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.DISABLE)), - enabledLive()); + liveInstances("h1")); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1"), "DISABLE instances cannot accept ONLINE replicas and must count regardless of liveness"); @@ -80,7 +81,7 @@ public void testDisableLiveIncluded() { public void testDisableOfflineIncluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.DISABLE)), - enabledLive()); + liveInstances()); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1")); } @@ -89,7 +90,7 @@ public void testDisableOfflineIncluded() { public void testEvacuateLiveIncluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.EVACUATE)), - enabledLive()); + liveInstances("h1")); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1"), "EVACUATE is the asymmetric op pre-fix; both entry and exit must now count it"); @@ -99,7 +100,7 @@ public void testEvacuateLiveIncluded() { public void testEvacuateOfflineIncluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.EVACUATE)), - enabledLive()); + liveInstances()); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1")); } @@ -108,7 +109,7 @@ public void testEvacuateOfflineIncluded() { public void testSwapInExcluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.SWAP_IN)), - enabledLive()); + liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty(), "SWAP_IN is in UNROUTABLE_INSTANCE_OPERATIONS and must never count"); } @@ -117,7 +118,7 @@ public void testSwapInExcluded() { public void testUnknownExcluded() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.UNKNOWN)), - enabledLive()); + liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty(), "UNKNOWN is in UNROUTABLE_INSTANCE_OPERATIONS and must never count"); } @@ -128,7 +129,7 @@ public void testUnknownExcluded() { public void testValidMarkerExemptsEnableOffline() { BaseControllerDataProvider provider = providerWith( configs(configWithMarker("h1", InstanceConstants.InstanceOperation.ENABLE, FUTURE_MS)), - enabledLive()); + liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty(), "Valid marker on ENABLE+offline must exempt the instance from the budget"); } @@ -137,7 +138,7 @@ public void testValidMarkerExemptsEnableOffline() { public void testValidMarkerExemptsEvacuate() { BaseControllerDataProvider provider = providerWith( configs(configWithMarker("h1", InstanceConstants.InstanceOperation.EVACUATE, FUTURE_MS)), - enabledLive()); + liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty(), "Valid marker on EVACUATE must exempt — this is the orchestrator-driven decom case"); } @@ -146,7 +147,7 @@ public void testValidMarkerExemptsEvacuate() { public void testValidMarkerExemptsDisable() { BaseControllerDataProvider provider = providerWith( configs(configWithMarker("h1", InstanceConstants.InstanceOperation.DISABLE, FUTURE_MS)), - enabledLive()); + liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty()); } @@ -154,7 +155,7 @@ public void testValidMarkerExemptsDisable() { public void testExpiredMarkerDoesNotExempt() { BaseControllerDataProvider provider = providerWith( configs(configWithMarker("h1", InstanceConstants.InstanceOperation.ENABLE, PAST_MS)), - enabledLive()); + liveInstances()); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1"), "Expired marker must behave as if the marker were absent"); } @@ -165,7 +166,7 @@ public void testBoundaryNowEqualsUntil() { // already past the window. BaseControllerDataProvider provider = providerWith( configs(configWithMarker("h1", InstanceConstants.InstanceOperation.ENABLE, NOW_MS)), - enabledLive()); + liveInstances()); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("h1"), "nowMs == untilMs is no longer under maintenance; instance must count"); } @@ -175,7 +176,7 @@ public void testMarkerOnSwapInIsIrrelevant() { // SWAP_IN is filtered out before the marker check; a marker on it must not change that. BaseControllerDataProvider provider = providerWith( configs(configWithMarker("h1", InstanceConstants.InstanceOperation.SWAP_IN, FUTURE_MS)), - enabledLive()); + liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty(), "Marker on a SWAP_IN instance must not change the (already excluded) outcome"); } @@ -184,7 +185,7 @@ public void testMarkerOnSwapInIsIrrelevant() { @Test public void testEmptyCluster() { - BaseControllerDataProvider provider = providerWith(configs(), enabledLive()); + BaseControllerDataProvider provider = providerWith(configs(), liveInstances()); Assert.assertTrue(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS).isEmpty()); } @@ -194,11 +195,11 @@ public void testMixedCluster() { // 1 DISABLE (counts), 1 EVACUATE (counts), 1 EVACUATE w/marker (exempt), // 1 SWAP_IN (excluded), 1 UNKNOWN (excluded). Map instanceConfigMap = new HashMap<>(); - Set enabledLive = new HashSet<>(); + Set liveInstanceNames = new HashSet<>(); for (int i = 0; i < 8; i++) { String name = "enable-live-" + i; instanceConfigMap.put(name, config(name, InstanceConstants.InstanceOperation.ENABLE)); - enabledLive.add(name); + liveInstanceNames.add(name); } instanceConfigMap.put("enable-offline", config("enable-offline", InstanceConstants.InstanceOperation.ENABLE)); @@ -217,7 +218,7 @@ public void testMixedCluster() { instanceConfigMap.put("unknown", config("unknown", InstanceConstants.InstanceOperation.UNKNOWN)); - BaseControllerDataProvider provider = providerWith(instanceConfigMap, enabledLive); + BaseControllerDataProvider provider = providerWith(instanceConfigMap, liveInstanceNames); Assert.assertEquals(provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS), setOf("enable-offline", "disable", "evacuate"), @@ -232,7 +233,7 @@ public void testReturnedSetIsModifiable() { // particular) rely on this for downstream filtering. Verify both properties hold. BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.ENABLE)), - enabledLive()); + liveInstances()); Set result = provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS); result.add("h99"); Assert.assertTrue(result.contains("h99"), "Returned set must be modifiable"); @@ -242,7 +243,7 @@ public void testReturnedSetIsModifiable() { public void testReturnedSetIsIndependentOfFutureCalls() { BaseControllerDataProvider provider = providerWith( configs(config("h1", InstanceConstants.InstanceOperation.ENABLE)), - enabledLive()); + liveInstances()); Set first = provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS); first.clear(); Set second = provider.getInstancesUnableToAcceptOnlineReplicas(NOW_MS); @@ -254,13 +255,18 @@ public void testReturnedSetIsIndependentOfFutureCalls() { /** * Builds a {@link BaseControllerDataProvider} whose only contract is to return the - * supplied instance-config map and enabled-live set. The accessor under test depends on + * supplied instance-config map and live-instance set. The accessor under test depends on * exactly those two hooks plus {@link InstanceConfig#isUnderInstanceOperationMaintenance}, * so overriding the two getters is sufficient and avoids initializing the full cluster - * cache machinery. + * cache machinery. The enabled-live subset is derived from the configs the same way the + * controller derives it, so the ENABLE filter is exercised here rather than stubbed. */ private static BaseControllerDataProvider providerWith( - Map instanceConfigMap, Set enabledLive) { + Map instanceConfigMap, Set liveInstanceNames) { + Map liveInstanceMap = new HashMap<>(); + for (String name : liveInstanceNames) { + liveInstanceMap.put(name, new LiveInstance(name)); + } return new BaseControllerDataProvider() { @Override public Map getInstanceConfigMap() { @@ -268,8 +274,8 @@ public Map getInstanceConfigMap() { } @Override - public Set getEnabledLiveInstances() { - return Collections.unmodifiableSet(enabledLive); + public Map getLiveInstances() { + return Collections.unmodifiableMap(liveInstanceMap); } }; } @@ -297,7 +303,7 @@ private static Map configs(InstanceConfig... cfgs) { } /** - * Test-side set builder. Used for both the enabled-live input set and the expected-result + * Test-side set builder. Used for both the live-instance input set and the expected-result * set so that assertions and setup share the same shape; the two readings sit at the call * site through the method name on the input side and explicit assertEquals on the result. */ @@ -307,7 +313,7 @@ private static Set setOf(String... names) { return set; } - private static Set enabledLive(String... names) { + private static Set liveInstances(String... names) { return setOf(names); } } diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/AbstractResource.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/AbstractResource.java index 9dc376f3e9..bdec3b858d 100644 --- a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/AbstractResource.java +++ b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/AbstractResource.java @@ -73,6 +73,7 @@ public enum Command { delete, stoppable, instanceOperationMaintenance, + getInstancesUnableToAcceptOnlineReplicas, rebalance, reset, resetPartitions, diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java index ee49118304..b4db8fcbe5 100644 --- a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java +++ b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java @@ -22,10 +22,12 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import java.util.stream.Collectors; import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; @@ -44,8 +46,10 @@ import org.apache.helix.HelixAdmin; import org.apache.helix.HelixDataAccessor; import org.apache.helix.HelixException; +import org.apache.helix.PropertyKey; import org.apache.helix.constants.InstanceConstants; import org.apache.helix.manager.zk.ZKHelixDataAccessor; +import org.apache.helix.manager.zk.ZKUtil; import org.apache.helix.model.ClusterConfig; import org.apache.helix.model.InstanceConfig; import org.apache.helix.rest.client.CustomRestClientFactory; @@ -61,6 +65,7 @@ import org.apache.helix.rest.server.resources.exceptions.HelixHealthException; import org.apache.helix.rest.server.service.ClusterService; import org.apache.helix.rest.server.service.ClusterServiceImpl; +import org.apache.helix.util.InstanceUtil; import org.apache.helix.util.InstanceValidationUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -87,7 +92,8 @@ public enum InstancesProperties { skip_stoppable_check_list, customized_values, instance_stoppable_parallel, - instance_not_stoppable_with_reasons + instance_not_stoppable_with_reasons, + instances_unable_to_accept_online_replicas } public enum InstanceHealthSelectionBase { @@ -124,7 +130,7 @@ public Response getAllInstances(@PathParam("clusterId") String clusterId, ArrayNode instancesNode = root.putArray(InstancesAccessor.InstancesProperties.instances.name()); instancesNode.addAll((ArrayNode) OBJECT_MAPPER.valueToTree(instances)); - + ArrayNode onlineNode = root.putArray(InstancesAccessor.InstancesProperties.online.name()); ArrayNode enabledNode = root.putArray(InstancesAccessor.InstancesProperties.enabled.name()); ArrayNode disabledNode = root.putArray(InstancesAccessor.InstancesProperties.disabled.name()); @@ -184,12 +190,89 @@ public Response getAllInstances(@PathParam("clusterId") String clusterId, return badRequest(e.getMessage()); } return JSONRepresentation(validationResultMap); + case getInstancesUnableToAcceptOnlineReplicas: + return getInstancesUnableToAcceptOnlineReplicas(clusterId, accessor); default: _logger.error("Unsupported command :" + command); return badRequest("Unsupported command :" + command); } } + /** + * Reports the instances Helix itself counts against the cluster-wide offline budget that + * drives auto Maintenance Mode, so clients do not have to reimplement (and drift from) the + * controller's membership rules. + * + *

The population is computed by + * {@link InstanceUtil#getInstancesUnableToAcceptOnlineReplicas(Map, java.util.Collection, long)}, + * the same method the controller uses on MM entry ({@code BestPossibleStateCalcStage}) + * and MM exit ({@code MaintenanceRecoveryStage}). An instance counts when it is routable, + * not enabled-and-live, and not covered by a valid instance-operation maintenance marker. + * + *

Response (HTTP 200), shaped like the {@code getAllInstances} response on this route: + * 

{@code
+   * { "id": "cluster0",
+   *   "instances_unable_to_accept_online_replicas": ["h3", "h4"] }
+   * }
+ * + *

Only the population is returned. The thresholds it is compared against + * ({@code MAX_OFFLINE_INSTANCES_ALLOWED}, {@code NUM_OFFLINE_INSTANCES_FOR_AUTO_EXIT}) are + * already available from the cluster-config endpoint, and the resulting maintenance state is + * available from the maintenance-signal endpoint; deriving either here would hand clients a + * prediction where an authoritative answer already exists. + */ + private Response + getInstancesUnableToAcceptOnlineReplicas(String clusterId, + HelixDataAccessor accessor) { + try { + return computeInstancesUnableToAcceptOnlineReplicas(clusterId, accessor); + } catch (Exception e) { + _logger.error("Failed to compute instances unable to accept online replicas for cluster {}", + clusterId, e); + return serverError(e); + } + } + + private Response computeInstancesUnableToAcceptOnlineReplicas(String clusterId, + HelixDataAccessor accessor) { + // An unknown cluster must not fall through to an empty population: to a client, "no instances + // counted" reads as "the whole offline budget is free". The caller's null check cannot catch + // it because HelixDataAccessor#getChildNames normalizes a missing path to an empty list, so + // resolve the cluster explicitly, the same way ClusterAccessor does. ConfigAccessor is not + // used here because it throws on an unknown cluster, which would surface as a 500. + if (!ZKUtil.isClusterSetup(clusterId, getRealmAwareZkClient())) { + return notFound(); + } + + PropertyKey.Builder keyBuilder = accessor.keyBuilder(); + List instanceConfigs = + accessor.getChildValues(keyBuilder.instanceConfigs(), true); + Map instanceConfigMap = new HashMap<>(); + if (instanceConfigs != null) { + for (InstanceConfig instanceConfig : instanceConfigs) { + if (instanceConfig != null) { + instanceConfigMap.put(instanceConfig.getInstanceName(), instanceConfig); + } + } + } + List liveInstances = accessor.getChildNames(keyBuilder.liveInstances()); + + Set unableToAcceptOnlineReplicas = + InstanceUtil.getInstancesUnableToAcceptOnlineReplicas(instanceConfigMap, + liveInstances == null ? Collections.emptyList() : liveInstances, + System.currentTimeMillis()); + + ObjectNode root = JsonNodeFactory.instance.objectNode(); + root.put(Properties.id.name(), clusterId); + ArrayNode countedNode = + root.putArray(InstancesProperties.instances_unable_to_accept_online_replicas.name()); + // Sorted so the payload is stable across calls for the same cluster state. + for (String instanceName : new TreeSet<>(unableToAcceptOnlineReplicas)) { + countedNode.add(instanceName); + } + return JSONRepresentation(root); + } + @ResponseMetered(name = HttpConstants.WRITE_REQUEST) @Timed(name = HttpConstants.WRITE_REQUEST) @POST diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java index 0fea71431e..79d34734de 100644 --- a/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java +++ b/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -971,6 +972,102 @@ public void testInstanceStoppableWithIncludeDetails() throws IOException { System.out.println("End test :" + TestHelper.getTestMethodName()); } + @Test + public void testGetInstancesUnableToAcceptOnlineReplicas() throws IOException { + System.out.println("Start test :" + TestHelper.getTestMethodName()); + + // Dedicated cluster so the population asserted here is not perturbed by other tests. No + // participants are started, so every routable instance is offline and counts against the + // budget unless it carries a valid instance-operation maintenance marker. + String clusterName = "TestOfflineBudgetCluster"; + _gSetupTool.addCluster(clusterName, true); + _clusters.add(clusterName); + List instances = + Arrays.asList("obInstance0", "obInstance1", "obInstance2", "obInstance3", "obInstance4"); + for (String instance : instances) { + _gSetupTool.addInstanceToCluster(clusterName, instance); + } + + // Baseline: all 5 instances are offline and unmarked, so all 5 count. + JsonNode node = OBJECT_MAPPER.readTree( + new JerseyUriRequestBuilder( + "clusters/{}/instances?command=getInstancesUnableToAcceptOnlineReplicas") + .isBodyReturnExpected(true).format(clusterName).get(this)); + Assert.assertEquals( + getSortedStringList(node, + InstancesAccessor.InstancesProperties.instances_unable_to_accept_online_replicas + .name()), sorted(instances)); + + // A valid marker exempts an instance; an expired marker does not. A SWAP_IN instance is + // never counted regardless of marker state. + long nowMs = System.currentTimeMillis(); + setInstanceOperationMaintenanceUntilMs(clusterName, "obInstance0", nowMs + 600_000L); + setInstanceOperationMaintenanceUntilMs(clusterName, "obInstance1", nowMs - 1L); + InstanceConfig swapInConfig = _configAccessor.getInstanceConfig(clusterName, "obInstance2"); + swapInConfig.setInstanceOperation(new InstanceConfig.InstanceOperation.Builder() + .setOperation(InstanceConstants.InstanceOperation.SWAP_IN).build()); + _configAccessor.setInstanceConfig(clusterName, "obInstance2", swapInConfig); + + node = OBJECT_MAPPER.readTree( + new JerseyUriRequestBuilder( + "clusters/{}/instances?command=getInstancesUnableToAcceptOnlineReplicas") + .isBodyReturnExpected(true).format(clusterName).get(this)); + Assert.assertEquals( + getSortedStringList(node, + InstancesAccessor.InstancesProperties.instances_unable_to_accept_online_replicas + .name()), + sorted(Arrays.asList("obInstance1", "obInstance3", "obInstance4")), + "Valid marker and SWAP_IN must be excluded; the expired marker must still count"); + + // The population is a property of the instances alone. Changing the budget thresholds the + // controller compares it against must not change who is in it. + ClusterConfig clusterConfig = _configAccessor.getClusterConfig(clusterName); + clusterConfig.setMaxOfflineInstancesAllowed(1); + clusterConfig.setNumOfflineInstancesForAutoExit(0); + _configAccessor.setClusterConfig(clusterName, clusterConfig); + node = OBJECT_MAPPER.readTree( + new JerseyUriRequestBuilder( + "clusters/{}/instances?command=getInstancesUnableToAcceptOnlineReplicas") + .isBodyReturnExpected(true).format(clusterName).get(this)); + Assert.assertEquals( + getSortedStringList(node, + InstancesAccessor.InstancesProperties.instances_unable_to_accept_online_replicas + .name()), + sorted(Arrays.asList("obInstance1", "obInstance3", "obInstance4")), + "Offline-budget thresholds must not affect which instances are counted"); + + // An unknown cluster must 404 like every other command on this route, not 500. Resolving the + // cluster through ConfigAccessor instead of the request's HelixDataAccessor would throw here. + new JerseyUriRequestBuilder( + "clusters/{}/instances?command=getInstancesUnableToAcceptOnlineReplicas") + .expectedReturnStatusCode(Response.Status.NOT_FOUND.getStatusCode()) + .format("TestOfflineBudgetClusterDoesNotExist").get(this); + + System.out.println("End test :" + TestHelper.getTestMethodName()); + } + + private void setInstanceOperationMaintenanceUntilMs(String clusterName, String instanceName, + long untilMs) { + InstanceConfig instanceConfig = _configAccessor.getInstanceConfig(clusterName, instanceName); + instanceConfig.setInstanceOperationMaintenanceUntilMs(untilMs); + _configAccessor.setInstanceConfig(clusterName, instanceName, instanceConfig); + } + + private static List sorted(Collection names) { + List result = new ArrayList<>(names); + Collections.sort(result); + return result; + } + + /** + * Reads a JSON array field as a sorted list. Sorting both sides keeps the comparison + * order-insensitive while still producing a readable diff on failure (TestNG compares + * collections element-by-element in iteration order). + */ + private List getSortedStringList(JsonNode jsonNode, String key) { + return sorted(getStringSet(jsonNode, key)); + } + private Set getStringSet(JsonNode jsonNode, String key) { Set result = new HashSet<>(); jsonNode.withArray(key).forEach(s -> result.add(s.textValue()));