Authoring rule #2: add a partition-weight guard rail (unplaceable-weight check) on the addWagedResource endpoint - #1
Closed
sjainit wants to merge 14 commits into
Conversation
Helix measures how long a partition's top state is missing but has no signal for how long a partition stays below its minActiveReplicas count. This adds a per-resource "partition recovery duration" metric, reusing the existing top-state-handoff timing infrastructure. New JMX metrics on ResourceMonitor (per resource): - PartitionRecoveryDurationGauge (histogram): end-to-end recovery duration, the headline/SLO metric. - PartitionRecoveryHelixLatencyGauge (histogram): Helix-controlled portion; registered but populated by a follow-up (v1 emits a negative latency that updatePartitionRecoveryStats skips). - PartitionsRecoveryDurationBeyondThresholdGauge: partitions currently past the recovery threshold; the alerting signal and the only metric that catches a partition that never heals. - SucceededPartitionRecoveryCounter: completed-recovery count / denominator. Detection is wired into TopStateHandoffReportStage.updateTopStateStatus as a per-partition edge detector over a new ResourceControllerDataProvider cache map: on dropping below minActiveReplicas the controller stamps a start time; on returning to the minimum it emits the elapsed duration and clears the record. The active replica count is read from CurrentStateOutput (the ExternalView is not computed at this stage), reusing the same active-state definition as ResourceMonitor.updateResourceState. The alert threshold is a new cluster-level config PARTITION_RECOVERY_DURATION_THRESHOLD (default 5m). Includes the design doc (docs/design/004) and unit tests covering record creation, recovery emission, beyond-threshold alerting, and cache lifecycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sjainit
force-pushed
the
sarjain/helix-rest-guardrail-weight-rule
branch
from
July 17, 2026 06:17
2d1b095 to
7a95001
Compare
sjainit
changed the title
sjainit
changed the title
sjainit
marked this pull request as ready for review
July 17, 2026 06:48
Rename PartitionsRecoveryDurationBeyondThresholdGauge to PartitionsRecoveryDurationBeyondThresholdCounter and change its semantics from a point-in-time up/down gauge to a monotonic counter: incremented once at recovery when the below-min window exceeded the threshold, and never decremented. A point-in-time gauge is only nonzero while a breach is in flight, so a breach that crosses the threshold and then heals between monitoring scrapes is invisible -- and fast-healing breaches (the shortest over-threshold tail) are the easiest to miss. A never-decremented counter makes breach occurrences reliably countable via increase()/rate() regardless of scrape timing. Partitions stuck below-min indefinitely remain covered by the existing point-in-time MissingMinActiveReplicaPartitionGauge, so there is no coverage gap. Removes the now-unused failed flag on MissingMinActiveReplicaRecord and the mid-flight increment / decrement-on-heal wiring. Updates unit tests and the design doc accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…raded-run assertions - Rename _successRecoveryCounter -> _succeededPartitionRecoveryCounter to match the emitted SucceededPartitionRecoveryCounter metric name. - Replace the two 'if (resourceMonitor != null)' silent guards in the degraded-run tests with explicit assertNull: a degraded-only pipeline run emits nothing, so no ResourceMonitor is created. Asserting null prevents a stray in-flight emission from passing green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback: - ResourceMonitor.updatePartitionRecoveryStats: guard the duration histogram with 'totalDuration >= 0'. totalDuration is a wall-clock diff across two pipeline runs; a backward clock step (NTP/PTP) can make it negative and corrupt the histogram min/p99/max. The recovery is still counted; only the invalid duration sample is skipped, mirroring the existing helixLatency guard. - TopStateHandoffReportStage.updatePartitionRecoveryStatus: skip disabled resources (idealState.isEnabled()), mirroring ResourceMonitor#updateResourceState. A disabled resource is not expected to maintain its replicas, so a drop below min while disabled is not a real recovery and must not be tracked or emitted. Tests: add testDisabledResourceCreatesNoRecord and a negative-duration case in testUpdatePartitionRecoveryStats. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A missing recovery record is ambiguous: it can mean the partition was healthy and just dropped, but it can also mean the partition has never been observed (a brand-new resource, a partition expansion, or the first pipeline run after clearMonitoringRecords() wipes the in-memory records on a leadership change). Opening a recovery window in the latter cases mis-counts partition bring-up as a recovery, permanently polluting the durable histogram and the monotonic beyond-threshold counter. Gate record creation on the previously published ExternalView, which is refreshed from ZooKeeper at the start of every pipeline run and is durable across leadership changes. Only open a recovery window when that ExternalView confirms the partition was at or above minActiveReplicas, so bring-up and pre-existing degradation are not mistaken for a drop. Refactor countActiveReplicas into a null-safe overload over an instance->state map so it can be reused for the ExternalView state map. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
While the cluster is in maintenance mode the controller intentionally holds off restoring or moving replicas (node swaps, take-downs, the maintenance-timeout window), so a partition sitting below minActiveReplicas is expected behavior rather than an availability regression. Counting it would inflate the recovery-duration histogram by the length of the maintenance window and fire false beyond-threshold breaches, defeating the purpose of the metric. Add cache.isMaintenanceModeEnabled() to the existing early return in updatePartitionRecoveryStatus (alongside the null/disabled checks), so below-min partitions are ignored cluster-wide while maintenance mode is active. This resolves the maintenance-mode open question deferred in the design doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
WorkflowMonitor exposed queued, running, and failed workflow gauges but had no metric for workflows sitting in the STOPPED state. A workflow (for example a stopped job queue) could remain STOPPED indefinitely without any observable signal, so operators had no way to alert on it and only noticed after downstream detection windows elapsed and logs had already rotated. Add a per-workflow-type StoppedWorkflowGauge that is refreshed each pipeline cycle alongside the existing gauges, cleared in resetGauges, and registered as an MBean attribute. Operators can now alert when any workflow of a given type is stopped. Extend TestWorkflowMonitor to cover the new gauge on both the accumulate and post-reset paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ry-time-metrics Add partition recovery time metrics
…live instance guard-rail rule (linkedin#213) * Add guard rail framework for helix-rest with min-active-replica rule Introduce a small, extensible pre-write validation framework in helix-core (org.apache.helix.guardrail) that lets helix-rest endpoints pre-validate a proposed cluster mutation before ZooKeeper is updated, so unsafe operations are rejected up front instead of surfacing later as rebalance failures. Framework (helix-core): - GuardrailRule: a pure, read-only check over a GuardrailContext. - GuardrailContext: immutable, builder-based carrier (cluster, data accessor, target instance) that grows additively as new rule types are added. - GuardrailPipeline: runs an endpoint's rules and aggregates violations; fails closed if a rule throws. Endpoints construct a pipeline with just the rules they need, so no central registry or per-operation dispatch is needed. - ValidationResult / Violation: immutable, JSON-serializable verdict types. Rule (helix-core): - MinActiveReplicaGuardrailRule: blocks dropping an instance when doing so would take any hosted partition below its configured minActiveReplicas, delegating to InstanceValidationUtil.siblingNodesActiveReplicaCheckWithDetails. REST wiring (helix-rest): - AbstractHelixResource.preflight(...) supports three modes from one read-only pipeline: enforce (400 with verdict), dryRun=true (200 verdict, never writes, i.e. "simulate"), and force=true (proceed despite violations, logged). - DELETE /clusters/{cluster}/instances/{instance} now runs the guard rail and accepts force and dryRun query params. Tests: - TestGuardrailPipeline: framework mechanics (aggregation, fail-closed, immutability). - TestMinActiveReplicaGuardrailRule: rule pass/fail/no-target via a mocked HelixDataAccessor. - TestPerInstanceAccessor: block (400), dryRun (200 verdict), and force-bypass paths for the delete endpoint. Also adds the design doc docs/design/005-helix-rest-guard-rails.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Delete docs/design/005-helix-rest-guard-rails.md * Address code-review findings on the guard rail framework Follow-up fixes from an independent review of the guard rail framework: - GuardrailPipeline: a rule that returns null now fails closed (records a violation + logs), symmetric with a rule that throws. Previously a null result was silently treated as feasible, contradicting the fail-closed policy and letting a misbehaving rule open the gate. - MinActiveReplicaGuardrailRule: catch HelixException from siblingNodesActiveReplicaCheckWithDetails (which scans every enabled resource and can throw, e.g. when an unrelated resource has no ExternalView yet, before it can tell whether the target instance even hosts it) and return an honest "could not verify minimum active replicas" violation with the force=true escape hatch, instead of mislabeling an evaluation failure as a replica shortfall. - AbstractHelixResource.preflight: document that a dry-run verdict reflects only the guard rail rules, not the full feasibility of the underlying mutation (which may enforce its own preconditions). - Tests: add a null-returning-rule fail-closed test to TestGuardrailPipeline; strengthen the force-bypass integration test to assert the request reached dropInstance (fails on the live participant with "is still alive") rather than merely returning some 400. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Scope min-active-replica guard rail to hosted resources; document preflight race Addresses review feedback on the guard rail framework. 1) MinActiveReplicaGuardrailRule previously delegated to siblingNodesActiveReplicaCheckWithDetails, which scans every enabled resource and throws the instant any resource lacks an ExternalView -- before it checks whether that resource is even hosted on the target instance. The rule caught that and failed closed, so a single not-yet-placed resource blocked DELETE for every instance in the cluster until its ExternalView was computed. Add a backward-compatible overload of siblingNodesActiveReplicaCheckWithDetails that skips resources without an ExternalView (a resource with no committed placement is not hosted on the instance and cannot be driven below its min active replicas by dropping it). The existing 3-arg method delegates with skip=false, so MaintenanceManagementService keeps its strict behavior. The rule uses skip=true, scoping the block to resources actually hosted on the instance. 2) Document in AbstractHelixResource.preflight that the verdict is computed from a state snapshot and the cluster can change between the preflight read and the actual mutation, so the check is a best-effort early abort rather than a transactional gate; the mutation's own preconditions and the controller remain the authoritative safety net. Adds a unit test asserting a resource without an ExternalView is skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Replace min-active-replica instance-drop guard rail with a live-instance check The DELETE instance guard rail previously ran MinActiveReplicaGuardrailRule, which scanned every resource's ideal state / external view to reject a drop that would breach min active replicas. On the actual delete path that check is both expensive and near-silent: an instance can only be dropped once it is already offline, at which point its at-risk partitions are skipped and the rule passes regardless of replication state. The meaningful precondition for dropping an instance is simply that it is no longer live. Replace it with LiveInstanceGuardrailRule, which reads only the target's single LIVEINSTANCES znode and blocks (or, for dryRun, reports) the drop while the participant is still connected. This mirrors the authoritative check already in ZKHelixAdmin.dropInstance but surfaces it as a structured pre-flight verdict and makes dryRun truthful, at negligible ZK read cost. The admin-layer check is retained as the enforcer for non-REST callers and to close the TOCTOU gap. - Add LiveInstanceGuardrailRule (+ unit test) - Wire it into PerInstanceAccessor.deleteInstance in place of the min-active rule - Update TestPerInstanceAccessor delete guard-rail coverage - Remove MinActiveReplicaGuardrailRule and its unit test The shared InstanceValidationUtil.siblingNodesActiveReplicaCheckWithDetails and MinActiveReplicaCheckResult helpers are left intact; they remain in use by MaintenanceManagementService. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Narrow guard rail data access to a read-only view Guard rail rules are contractually pure and read-only, but GuardrailContext handed them a full HelixDataAccessor, which also exposes setProperty / updateProperty / removeProperty / createChildren. That let a validation rule mutate the cluster -- a footgun the contract forbids but the type permitted. Introduce ReadOnlyDataAccessor, a narrow interface exposing only the read subset (getProperty, getChildNames, getChildValues, keyBuilder), and hand that to rules instead. GuardrailContext.Builder.dataAccessor(...) still accepts a HelixDataAccessor and wraps it via ReadOnlyDataAccessor.of(...), so REST callers are unchanged; only rules see the narrowed type, enforcing the read-only contract at compile time rather than by convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Sarthak Jain <sarjain@linkedin.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Block adding a WAGED resource whose per-partition weight exceeds the largest single instance's capacity in any dimension, which would make the partition permanently unplaceable. Existing addWagedResource validation only checks that weight keys are present, never their magnitude, so today such a resource is accepted into ZooKeeper and only fails later at rebalance time. This rule closes that gap by pre-validating the mutation on the REST endpoint. - New PartitionWeightCapacityGuardrailRule computes, per capacity dimension, the maximum capacity advertised by any single instance and fails the mutation when a partition's effective weight exceeds it. - GuardrailContext carries the proposed ResourceConfig so rules can read the to-be-written weights before the object exists in ZK. - ResourceAccessor.addResource wires the rule into the addWagedResource path with force/dryRun, mirroring the existing instance-drop guard rail. - Unit tests for the rule plus an integration test for the endpoint (enforce, dry-run, force bypass, within-capacity happy path). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sjainit
force-pushed
the
sarjain/helix-rest-guardrail-weight-rule
branch
from
August 4, 2026 10:32
7a95001 to
73a59ef
Compare
Owner
Author
|
Superseded by linkedin#219. The guard rail framework (linkedin#213) has merged into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A stacked demo PR on top of the guard rail framework (linkedin#213). It exists to show, concretely, how a new rule is authored on the framework — the diff here is only the incremental change: one rule, one additive context field, one endpoint wired, and tests.
Stacked on:
sarjain/helix-rest-guardrail-framework(the head branch of linkedin#213). It must merge into that branch (or land after linkedin#213), hence Draft.The new rule:
PartitionWeightCapacityGuardrailRuleWhat it checks: for a WAGED resource, WAGED places each partition replica on exactly one instance, so a partition can only ever be placed if — for every capacity dimension
d— some instance hascapacity_d >= weight_d. This rule rejectsaddWagedResourcewhen a partition's effective weight in any dimension exceeds the largest single instance's capacity in that dimension, because such a partition is permanently unplaceable no matter how the cluster is arranged.Gap it closes: today
addWagedResourceonly validates that the required weight keys are present (WagedValidationUtil.validateAndGetPartitionCapacitychecks key coverage, never magnitude). So a resource whose weight is larger than any instance can hold is accepted, the write succeeds, and it only surfaces later as a WAGED rebalance failure. This rule compares magnitude against real instance capacity on the write path, so the impossible resource is rejected before it reaches ZooKeeper.It is deliberately a necessary (not sufficient) condition — it compares each dimension independently against the best instance in that dimension, so it never blocks a resource that could plausibly be placed; it only fails the provably-impossible cases. It is a no-op for non-WAGED clusters (no capacity keys), honors cluster-level default instance/partition weights (merged in the same way the rebalancer does), and evaluates the
DEFAULTpartition so resources relying purely on default weights are still checked.How it demonstrates the framework's extension points
GuardrailRule: null-guard → read what it needs from the context → reuse the same weight/capacity primitives WAGED uses → return aValidationResult(feasible, or an infeasible verdict with an actionable message). MirrorsMinActiveReplicaGuardrailRule.GuardrailContext.proposedResourceConfigis added via theBuilder. Existing rules/endpoints are untouched — this is the intended extensibility seam.ResourceAccessor.addResource(theaddWagedResourcecommand) with the sameforce/dryRunpreflight the delete endpoint already uses. No registry, no dispatch — the endpoint just buildsnew GuardrailPipeline(new PartitionWeightCapacityGuardrailRule()).Behavior
PUT /clusters/{cluster}/resources/{resource}?command=addWagedResource(body = map of IdealState + ResourceConfig ZNRecords):400+ JSON verdict, resource not created; within-capacity →200, created.dryRun=true): always200+ verdict, resource never created (validation-probe semantics, consistent with the framework).force=true): proceeds even for an over-weight resource →200, created, with the overridden verdict logged.Tests
TestPartitionWeightCapacityGuardrailRule, 8): null proposed config, null cluster config, no capacity keys (non-WAGED), no instance capacity, weight within capacity, weight exceeding capacity (DEFAULT → unscoped), per-partition override exceeding, and max-across-instances used. MockedHelixDataAccessor+ realClusterConfig/InstanceConfig/ResourceConfig.TestResourceAccessor#testAddWagedResourceWeightGuardrail, 1): full Jersey + embedded ZK, exercising enforce / dryRun / force / within-capacity with status codes, the JSON verdict, and actual resource presence/absence. Saves and restores the cluster + instance capacity configuration intry/finallyso it does not disturb sibling tests.Validated on JDK 11: 8 new rule unit tests (plus the existing framework unit tests) and the new REST integration test pass.
Not in scope
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com