Skip to content

[controller] Throttle parent-controller version-creation attempts - #2954

Open
pthirun wants to merge 10 commits into
linkedin:mainfrom
pthirun:pthirunavukkarasu/push-retry-cooldown
Open

[controller] Throttle parent-controller version-creation attempts#2954
pthirun wants to merge 10 commits into
linkedin:mainfrom
pthirun:pthirunavukkarasu/push-retry-cooldown

Conversation

@pthirun

@pthirun pthirun commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem Statement

Rapid retries with different push IDs can repeatedly reach version creation after an earlier attempt fails before any durable Version metadata or resources exist. Venice needs a best-effort abuse/throttling guard without adding durable store state, schema changes, ZK nodes, or admin protocol fields.

Solution

  • Add the cluster-level controller.push.retry.cooldown.ms config with a default of 600000 ms (10 minutes); 0 disables the mechanism.
  • Enforce only in the leader parent-controller version-creation path. The state is intentionally process-local, so parent-controller restart or leadership handoff starts with a fresh cooldown map.
  • Apply only to user-store BATCH and STREAM_REPROCESSING requests that are about to create a new version. Venice system stores, STREAM, INCREMENTAL, explicit-version child/admin processing, and migration replication are excluded.
  • Preserve request ordering: ongoing-push handling and existing-version same-push-ID idempotency run first, followed by degraded-mode and target-region validation. Invalid or rejected requests do not enter the map.
  • Atomically check and record a ConcurrentHashMap entry keyed by cluster/store immediately before addVersionAndTopicOnly(), before lifecycle hooks, version metadata, topics, push monitoring, or Helix resources.
  • Record the first admitted attempt; allow the same push ID without sliding its timestamp; reject a different push ID inside the window with HTTP 429 and remaining time; replace expired entries. Rejections do not slide the window, while admitted attempts remain recorded if later creation fails.
  • Remove an entry after successful parent-level store deletion. Other stale entries expire lazily.
  • Record a status-neutral cooldown rejection counter by push type.

Code changes

  • Added new code behind a config. Config: controller.push.retry.cooldown.ms; default: 600000 ms.
  • Introduced new log lines.
    • Confirmed if logs need to be rate limited to avoid excessive logging. No new log lines were added.

Concurrency-Specific Checks

Both reviewer and PR author to verify

  • Code has no race conditions or thread safety issues. Admission for each cluster/store key is serialized with ConcurrentHashMap.compute.
  • Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed. No additional lock is required beyond the atomic map operation.
  • No blocking calls inside critical sections that could lead to deadlocks or performance degradation.
  • Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList).
  • Validated proper exception handling in multi-threaded code to avoid silent thread termination.

How was this PR tested?

  • New unit tests added.
  • New integration tests added.
  • Modified or extended existing tests.
  • Verified backward compatibility (if applicable).
  • Local code review completed.

Commands run successfully:

  • ./gradlew :services:venice-controller:test --tests com.linkedin.venice.controller.TestVeniceParentHelixAdmin --tests com.linkedin.venice.controller.TestVeniceControllerClusterConfig --tests com.linkedin.venice.controller.stats.VeniceAdminOtelMetricEntityTest --tests com.linkedin.venice.controller.stats.VeniceAdminStatsOtelTest --tests com.linkedin.venice.controller.stats.VeniceAdminTehutiMetricNameEnumTest --no-daemon
  • ./gradlew :internal:venice-test-common:integrationTest --tests com.linkedin.venice.controller.VeniceParentHelixAdminTest.testPushRetryCooldownThroughControllerApi --no-daemon
  • ./gradlew :internal:venice-test-common:integrationTest --tests com.linkedin.venice.controller.VeniceParentHelixAdminTest.testAddVersion --no-daemon
  • ./gradlew spotlessCheck --no-daemon

The focused integration test uses a deterministic failing pre-version lifecycle hook, verifies the first admitted attempt creates no Version, version topic, or child Helix resource, verifies a different push ID receives HTTP 429 before resources, then removes the failure and confirms the original push ID succeeds.

Does this PR introduce any user-facing or breaking changes?

  • No. You can skip the rest of this section.
  • Yes. Clearly explain the behavior change and its impact.

When enabled, a new push ID for a user-store BATCH or STREAM_REPROCESSING version-creation attempt can receive HTTP 429 during the configured process-local cooldown. Same-push-ID retries remain idempotently admissible. The guard does not survive parent-controller restart or leadership handoff by design, and no durable metadata or schemas change.

🤖 Generated with GitHub Copilot CLI

@pthirun
pthirun marked this pull request as ready for review August 5, 2026 18:38
@pthirun pthirun changed the title [controller] Add failed-push retry cooldown [controller] Enforce cooldown between version-creating pushes Aug 10, 2026
@pthirun
pthirun marked this pull request as draft August 10, 2026 22:05
@pthirun
pthirun marked this pull request as ready for review August 11, 2026 08:41
pthirun and others added 8 commits August 12, 2026 12:02
Prevent rapid version creation after a failed full push while preserving
idempotent retries and successful push behavior. The cooldown uses persisted
version creation timestamps so controller failover does not reset enforcement.

Co-authored-by: Copilot <[email protected]>
Apply cluster-level spacing to every user-store batch or stream-reprocessing
version creation using persisted creation time. Preserve same-push ID
idempotency and existing ongoing-push handling while excluding system stores
and non-version-creating pushes.

Rename the config, policy, metrics, messages, and tests to status-neutral
terminology.

Co-authored-by: Copilot <[email protected]>
Reserve user-store version-creation attempts in Store metadata before
lifecycle hooks, topics, or version resources are created. This keeps the
fixed cooldown window durable across controller failover and failed creation
attempts while preserving same-push idempotent recovery.

Use the latest persisted Version creation time as an upgrade fallback, keep
explicit-version and non-batch paths excluded, and add model, policy,
controller, and integration coverage.

Co-authored-by: Copilot <[email protected]>
@pthirun
pthirun force-pushed the pthirunavukkarasu/push-retry-cooldown branch from b2b128f to b7650b4 Compare August 12, 2026 20:30
@pthirun
pthirun marked this pull request as draft August 12, 2026 20:31
@pthirun pthirun changed the title [controller] Enforce cooldown between version-creating pushes [controller] Persist version-creation attempt cooldown Aug 12, 2026
@pthirun
pthirun marked this pull request as ready for review August 12, 2026 21:42
@pthirun pthirun changed the title [controller] Persist version-creation attempt cooldown [controller] Enforce cooldown between version-creating pushes Aug 12, 2026
@pthirun
pthirun marked this pull request as draft August 12, 2026 22:09
@pthirun
pthirun marked this pull request as ready for review August 12, 2026 22:12
Use an atomic process-local cooldown in the parent controller immediately
before version creation. This limits rapid different-ID retries while
preserving same-ID idempotency and avoiding durable metadata changes.

Co-authored-by: Copilot <[email protected]>
@pthirun pthirun changed the title [controller] Enforce cooldown between version-creating pushes [controller] Throttle parent-controller version-creation attempts Aug 13, 2026
@pthirun
pthirun marked this pull request as draft August 13, 2026 21:59
@pthirun
pthirun marked this pull request as ready for review August 13, 2026 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants