Skip to content

[CELEBORN-2387] Add ReserveSlotsFailCount worker metric#3765

Open
shellfish007 wants to merge 2 commits into
apache:mainfrom
shellfish007:upstream-pr/reserve-slots-fail-count-metric
Open

[CELEBORN-2387] Add ReserveSlotsFailCount worker metric#3765
shellfish007 wants to merge 2 commits into
apache:mainfrom
shellfish007:upstream-pr/reserve-slots-fail-count-metric

Conversation

@shellfish007

@shellfish007 shellfish007 commented Jul 23, 2026

Copy link
Copy Markdown

What changes were proposed in this pull request?

Adds a new worker metric, ReserveSlotsFailCount, and increments it on every non-success path of Controller.handleReserveSlots.

Why are the changes needed?

handleReserveSlots already replies with distinct non-success status codes:

  • WORKER_SHUTDOWN — worker is shutting down
  • NO_AVAILABLE_WORKING_DIR — no healthy local dirs and no remote storage
  • RESERVE_SLOTS_FAILED — not all primary partitions could be satisfied
  • RESERVE_SLOTS_FAILED — not all replica partitions could be satisfied

...but none of these paths incremented a counter. Slot reservation was the only major worker operation without a *FailCount metric (push, replicate, fetch, commit, register all have one), so failed reservations were invisible to metrics/dashboards and only visible via logs.

WorkerSource.scala declares RESERVE_SLOTS_FAIL_COUNT = "ReserveSlotsFailCount" and registers it with addCounter(...), alongside SlotsAllocated. Controller.scala calls workerSource.incCounter(WorkerSource.RESERVE_SLOTS_FAIL_COUNT) before each of the four non-success context.reply(...) sites in handleReserveSlots.

One fail counter per failed reservation regardless of cause, matching the existing *FailCount convention (e.g. CommitFilesFailCount, RegisterWithMasterFailCount). This gives a single "reserve slots did not succeed" rate; the specific cause remains distinguishable via the status code in logs.

Does this PR introduce any user-facing change?

Yes — adds a new worker metric ReserveSlotsFailCount. No config or API changes.

How was this patch tested?

Follows the established counter pattern exactly (constant + addCounter registration + incCounter at the failure path), with no new imports or signatures.

handleReserveSlots already replies with distinct non-success status
codes (WORKER_SHUTDOWN, NO_AVAILABLE_WORKING_DIR, RESERVE_SLOTS_FAILED
for primary/replica), but none of these paths incremented a counter.
Slot reservation was the only major worker operation without a
*FailCount metric (push, replicate, fetch, commit, register all have
one), so failed reservations were invisible to dashboards and only
visible via logs.

Add WorkerSource.RESERVE_SLOTS_FAIL_COUNT and increment it on every
non-success reply path in Controller.handleReserveSlots, following the
existing *FailCount convention.
@shellfish007 shellfish007 changed the title Add ReserveSlotsFailCount worker metric [CELEBORN-2387] Add ReserveSlotsFailCount worker metric Jul 23, 2026
@SteNicholas
SteNicholas requested a review from Copilot July 23, 2026 07:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds a new worker-side metric counter to make slot reservation failures visible in metrics/dashboards by incrementing a ReserveSlotsFailCount counter on non-success reply paths in Controller.handleReserveSlots.

Changes:

  • Register a new worker metric counter ReserveSlotsFailCount in WorkerSource.
  • Increment ReserveSlotsFailCount on each non-success response path in Controller.handleReserveSlots.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/WorkerSource.scala Declares and registers the new ReserveSlotsFailCount counter.
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala Increments ReserveSlotsFailCount before returning non-success ReserveSlotsResponse results.
Comments suppressed due to low confidence (1)

worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala:248

  • RESERVE_SLOTS_FAIL_COUNT is incremented after destroyWriters(...). If cleanup throws, the metric won't be recorded and the RPC reply may never be sent. Increment the fail counter before cleanup so reservation failures are still counted even when cleanup fails.
      destroyWriters(primaryLocs, shuffleKey)
      destroyWriters(replicaLocs, shuffleKey)
      workerSource.incCounter(WorkerSource.RESERVE_SLOTS_FAIL_COUNT)
      context.reply(ReserveSlotsResponse(StatusCode.RESERVE_SLOTS_FAILED, msg))
      return

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 221 to 225
logWarning(s"[handleReserveSlots] $msg, will destroy writers.")
destroyWriters(primaryLocs, shuffleKey)
workerSource.incCounter(WorkerSource.RESERVE_SLOTS_FAIL_COUNT)
context.reply(ReserveSlotsResponse(StatusCode.RESERVE_SLOTS_FAILED, msg))
return
Ensures the failure is counted even if destroyWriters throws, per review feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants