Add endpoint for Helix-computed offline instance count - #225
Add endpoint for Helix-computed offline instance count#225bellatrix007 wants to merge 3 commits into
Conversation
Clients that respect MAX_OFFLINE_INSTANCES_ALLOWED had no way to ask Helix how many instances it actually counts against that budget, so they reimplemented the membership rules against a pinned copy of the controller source. That copy silently drifts whenever the rules change; the instance-operation maintenance marker is one such change, and a client without it over-counts and throttles itself against instances Helix has already exempted. Extract the offline-budget computation into InstanceUtil so the controller and helix-rest share one definition, and expose it read-only so clients can fetch the number Helix uses instead of deriving their own. Also report the exempted instances and the configured thresholds so a caller can explain a decision without a second round of requests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a read-only helix-rest endpoint that exposes the controller’s “offline budget” membership computation (instances unable to accept ONLINE replicas), aiming to prevent client-side reimplementation drift and reduce expensive multi-call client calculations.
Changes:
- Added
GET /clusters/{clusterId}/instances?command=getInstancesUnableToAcceptOnlineReplicasto return the computed instance set, count, and related thresholds. - Centralized the offline-budget computation in
helix-coreInstanceUtil, and updatedBaseControllerDataProviderto delegate to it. - Added/updated unit tests in helix-rest and helix-core to cover marker/liveness/operation-state behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java | Adds an integration-style test for the new instances “offline budget” endpoint response fields and sorting. |
| helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java | Implements the new getInstancesUnableToAcceptOnlineReplicas command handler and response payload. |
| helix-rest/src/main/java/org/apache/helix/rest/server/resources/AbstractResource.java | Adds the new command enum value to route requests. |
| helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestInstancesUnableToAcceptOnlineReplicas.java | Updates tests to stub live instances (not enabled-live), ensuring ENABLE filtering is exercised. |
| helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java | Introduces shared utility methods for enabled-live computation, offline-budget set computation, and marker set extraction. |
| helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java | Refactors offline-budget computation to delegate to InstanceUtil for a single source of truth. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private Response computeInstancesUnableToAcceptOnlineReplicas(String clusterId, | ||
| HelixDataAccessor accessor) { | ||
| ClusterConfig clusterConfig = getConfigAccessor().getClusterConfig(clusterId); | ||
| if (clusterConfig == null) { | ||
| return notFound(); | ||
| } | ||
|
|
||
| PropertyKey.Builder keyBuilder = accessor.keyBuilder(); | ||
| List<InstanceConfig> instanceConfigs = | ||
| accessor.getChildValues(keyBuilder.instanceConfigs(), true); |
There was a problem hiding this comment.
Good catch, and it turned out to be worse than a 500 — fixed in 639d1ae.
I first tried the suggested fix (resolve the cluster through the request's HelixDataAccessor instead of ConfigAccessor), but that doesn't work: ZKHelixDataAccessor#getChildNames normalizes a missing path to Collections.emptyList() and never returns null. So the instances == null guard before the command switch is effectively dead code, and an unknown cluster fell through to HTTP 200 with an empty list rather than the 500 you predicted.
For this endpoint specifically that's the most dangerous possible response: an empty population is indistinguishable from a healthy cluster, and a client reads it as "the entire offline budget is free" — exactly the failure mode this endpoint exists to prevent.
Now resolving the cluster explicitly with ZKUtil.isClusterSetup(...), the idiom ClusterAccessor#doesClusterExist already uses, and returning 404. ConfigAccessor is gone from this path, so there's no second ZK read path and no throw-on-unknown-cluster. Added a regression assertion pinning the 404.
Worth noting separately: getAllInstances on this same route has the same dead null guard and also 200s on an unknown cluster. Pre-existing, so I've left it out of this PR.
| Set<String> unableToAcceptOnlineReplicas = | ||
| InstanceUtil.getInstancesUnableToAcceptOnlineReplicas(instanceConfigMap, | ||
| liveInstances == null ? Collections.emptyList() : liveInstances, nowMs); | ||
| Set<String> underMaintenance = | ||
| InstanceUtil.getInstancesUnderInstanceOperationMaintenance(instanceConfigMap, nowMs); |
There was a problem hiding this comment.
Agreed — the field was claiming to answer "why isn't this instance counted?" while actually returning "every instance carrying a valid marker", which includes instances that were never in the population to begin with (ENABLE+live, or UNROUTABLE ops like SWAP_IN/UNKNOWN).
Rather than narrow it to the true exempted set, I removed it in 702db77, along with the other derived fields. The same review pass questioned why the response carried anything beyond the population at all, and the answer was that none of it earned a place in the API:
..._countislist.size(), and the siblinggetAllInstancescommand on this route returns bare arrays with no counts.max_offline_instances_allowed/num_offline_instances_for_auto_exitare already served by the cluster-config endpoint.exceeds_max_offline_instances_allowedpredicted whether the controller would auto-enter maintenance mode, when the actual maintenance state is authoritative and already available from the maintenance-signal endpoint. Shipping a prediction next to the real answer invites clients to act on the wrong one.instances_under_instance_operation_maintenanceis your comment: the markers are onInstanceConfigand readable per instance.
The response is now the cluster id plus the population, mirroring getAllInstances so both commands on the route share an envelope shape. InstanceUtil#getInstancesUnderInstanceOperationMaintenance lost its only caller and was removed, and getEnabledLiveInstances is now private — neither should ship as public API without a consumer.
The response also carried the count, the instances under an instance-operation maintenance marker, both budget thresholds, and a boolean saying whether the limit was exceeded. None of it earns a place in the API. The count is list.size(), and the sibling getAllInstances command on this same route returns bare arrays with no counts. The thresholds are already on the cluster-config endpoint. The maintenance markers are on InstanceConfig, readable per instance. And exceeds_max_offline_instances_allowed predicted whether the controller would auto-enter maintenance mode when the actual maintenance state is authoritative and already served by the maintenance-signal endpoint; shipping a prediction alongside the real answer invites clients to act on the wrong one. Every field is a permanent forward-compatibility commitment, and each of these existed for a speculative consumer. What remains is the cluster id and the population, mirroring getAllInstances so both commands on the route share an envelope. InstanceUtil#getInstancesUnderInstanceOperationMaintenance loses its only caller with the maintenance list and is removed; getEnabledLiveInstances is now used only within InstanceUtil and becomes private. Neither should ship as public API without a consumer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:221
- The PR description documents a richer response (count, thresholds, under-maintenance set, and breach boolean), but this endpoint currently returns only {id, instances_unable_to_accept_online_replicas} and the javadoc explicitly says “Only the population is returned”. Please align the public contract by either updating the PR description (and any external docs) to match this minimal payload, or expanding the response here to include the documented fields so clients don’t need additional calls.
* <p>Response (HTTP 200), shaped like the {@code getAllInstances} response on this route:
* <pre>{@code
* { "id": "cluster0",
* "instances_unable_to_accept_online_replicas": ["h3", "h4"] }
* }</pre>
*
* <p>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.
helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java:1062
- This helper sorts the JSON array before comparing, which makes the test order-insensitive and won’t catch regressions if the endpoint stops returning a stable sorted payload (a stated contract in the implementation). Consider asserting the array is already sorted instead of sorting it in the test.
/**
* 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<String> getSortedStringList(JsonNode jsonNode, String key) {
The endpoint resolved the cluster with ConfigAccessor#getClusterConfig, which throws rather than returning null when the cluster is not set up, so an unknown clusterId surfaced as a 500. It also opened a second ZK read path alongside the request's HelixDataAccessor. The route's existing guard cannot cover this: HelixDataAccessor#getChildNames normalizes a missing path to an empty list, never null, so the `instances == null` check before the command switch is dead code and an unknown cluster falls through to an empty result. An empty population is the worst answer this endpoint can give. It is indistinguishable from a healthy cluster, and a client reads it as "the entire offline budget is free" — the failure mode this endpoint exists to prevent. Resolve the cluster explicitly with ZKUtil.isClusterSetup, the idiom ClusterAccessor already uses, and 404 when it is absent. Reported by review on linkedin#225. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:216
- The PR description specifies this endpoint returns additional fields (count, under-maintenance set, offline-budget thresholds, and an
exceeds_max_offline_instances_allowedboolean), but the implementation/javadoc currently documents and returns only theinstances_unable_to_accept_online_replicaslist. This is a behavior/contract mismatch that will confuse API consumers; please align the endpoint response + tests with the described contract, or update the PR description/javadoc to match the intended minimal payload.
* <p>Response (HTTP 200), shaped like the {@code getAllInstances} response on this route:
* <pre>{@code
* { "id": "cluster0",
* "instances_unable_to_accept_online_replicas": ["h3", "h4"] }
* }</pre>
Issues
Follow-up to review feedback on #175: https://github.com/linkedin/helix/pull/175/changes#r3235901682
Description
What. Adds a read-only endpoint that reports the number of instances Helix itself counts against the cluster-wide offline budget driving auto Maintenance Mode:
Why. Requested in review on #175:
This is not just a convenience. Clients that respect
MAX_OFFLINE_INSTANCES_ALLOWEDcurrently reimplement the controller's membership rules against a pinned copy of Helix source. A concrete example is helixacm-service, whoseHelixRestClient.getAllInstancesUnableToTakeOnlineReplicasOrThrowcarries a comment pinning a Helix commit SHA and a line number inBestPossibleStateCalcStage. That copy applies the pre-#175 rules and has no notion of the instance-operation maintenance marker, so instances that Helix exempts are still counted by the client. The client's remaining quota comes out smaller than Helix's, and it throttles exactly the planned operations #175 was written to unblock. The client cannot detect this drift; nothing fails loudly.Cost also matters: that client makes
getLiveInstances+getAllInstances+ N per-instance config reads on every pipeline iteration (it carries its ownTODO: change this to batch get from ZK). This endpoint replaces all of it with one call.How.
InstanceUtilgains the offline-budget computation as the single definition of the rule:getInstancesUnableToAcceptOnlineReplicas(instanceConfigMap, liveInstanceNames, nowMs)— routable, not enabled-and-live, no valid instance-operation maintenance marker.getInstancesUnderInstanceOperationMaintenance(instanceConfigMap, nowMs)— the exempted set.getEnabledLiveInstances(instanceConfigMap, liveInstanceNames)— extracted so the ENABLE+live rule is stated once.BaseControllerDataProvider#getInstancesUnableToAcceptOnlineReplicas(long)now delegates toInstanceUtil. Behavior is unchanged; the point is that MM entry (BestPossibleStateCalcStage), MM exit (MaintenanceRecoveryStage), and the new endpoint can no longer drift from one another, which is the failure mode the review comment describes.helix-rest exposes it as a new
Commandon the existingGET /clusters/{c}/instancesrouter, alongsidegetAllInstancesandvalidateWeight.Response (HTTP 200):
{ "id": "cluster0", "instances_unable_to_accept_online_replicas": ["h3", "h4"], "instances_unable_to_accept_online_replicas_count": 2, "instances_under_instance_operation_maintenance": ["h1"], "max_offline_instances_allowed": 4, "num_offline_instances_for_auto_exit": 2, "exceeds_max_offline_instances_allowed": false }The instance lists are returned sorted so the payload is stable across calls for the same cluster state. The exempted set and both thresholds are included so a caller can explain a throttling decision without a second round of requests.
exceeds_max_offline_instances_allowedis always false whenmax_offline_instances_allowedis negative, matching the controller, which never auto-enters MM for this reason when the threshold is unset.Tests
helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java—testGetInstancesUnableToAcceptOnlineReplicas, on a dedicated cluster so the counts are not perturbed by other tests. Covers the unmarked baseline (all instances counted, limit exceeded), a valid marker exempting an instance, an expired marker still counting,SWAP_INexcluded, the reported under-maintenance set, and an unsetMAX_OFFLINE_INSTANCES_ALLOWEDnever reporting a breach.helix-core/src/test/java/org/apache/helix/controller/dataproviders/TestInstancesUnableToAcceptOnlineReplicas.java— updated to stub live instances rather than the derived enabled-live set, so the ENABLE filter is now exercised by these tests instead of being mocked out. All existing cases retained.Changes that Break Backward Compatibility (Optional)
None. The endpoint is additive (a new
Commandenum value on an existing route) and read-only. TheBaseControllerDataProviderchange is a pure extraction: the method keeps its signature and returns the same fresh modifiable set, and the maintenance-mode integration tests confirm entry and exit behavior is unchanged.Documentation (Optional)
Endpoint contract and the rationale for reporting the thresholds alongside the count are documented in the javadoc on
InstancesAccessor#getInstancesUnableToAcceptOnlineReplicas. The membership rules are documented once, onInstanceUtil#getInstancesUnableToAcceptOnlineReplicas.Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)