Skip to content

[core][raylet] Fix spill_manager_objects_bytes reports restored object count instead of restored bytes - #65013

Merged
rueian merged 1 commit into
ray-project:masterfrom
RocMarshal:ISSUE-65012
Jul 31, 2026
Merged

[core][raylet] Fix spill_manager_objects_bytes reports restored object count instead of restored bytes#65013
rueian merged 1 commit into
ray-project:masterfrom
RocMarshal:ISSUE-65012

Conversation

@RocMarshal

@RocMarshal RocMarshal commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

  • What problem does this solve?

    The spill_manager_objects_bytes metric appears to report the wrong value for
    State="Restored".

    In LocalObjectManager::RecordMetrics(), the restored sample for
    spill_manager_objects_bytes is recorded with restored_objects_total_:

      spill_manager_metrics_.spill_manager_objects_bytes_gauge.Record(                                                                                                                                            
      restored_objects_total_, {{"State", "Restored"}});

    However, this metric is documented as a byte-size metric:
    name = "spill_manager_objects_bytes"
    description = "Byte size of local objects broken per state ..."

  • How does this PR solve it?

    correct the new value of the target metrics .

  • Any relevant context for reviewers such as:

    [core][raylet] spill_manager_objects_bytes reports restored object count instead of restored bytes #65012

  • Why is the problem important to solve?

    IIUC, It's a but for metrics on the corresponding value.

  • Why was this approach chosen over others?

    direct & simple.

Related issues

Fixes #65012

Additional information

N.A

@RocMarshal
RocMarshal requested a review from a team as a code owner July 26, 2026 14:38
cursor[bot]
cursor Bot reviewed Jul 26, 2026
View reviewed changes

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 540d2d3. Configure here.

Comment thread src/ray/raylet/local_object_manager.cc Resolved
@RocMarshal

RocMarshal commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi, @edoakes @Sparks0219 Could you help take a look if you had the free time ?

Thank you very much!

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 26, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request corrects a metric recording bug in LocalObjectManager where restored_objects_total_ was incorrectly used instead of restored_bytes_total_ for recording restored bytes. It also adds a unit test to verify this behavior. The review feedback points out an accidental indentation change in the modified line that should be restored to maintain code consistency.

Comment thread src/ray/raylet/local_object_manager.cc Resolved
RocMarshal
RocMarshal commented Jul 26, 2026
View reviewed changes
Comment thread src/ray/raylet/local_object_manager.cc Resolved
@RocMarshal RocMarshal changed the title [core][raylet] Fix spill_manager_objects_bytes reports restored objec… [core][raylet] Fix spill_manager_objects_bytes reports restored object count instead of restored bytes Jul 26, 2026
@ray-gardener ray-gardener Bot added core Issues that should be addressed in Ray Core community-contribution Contributed by the community labels Jul 26, 2026
@RocMarshal RocMarshal closed this Jul 27, 2026
@RocMarshal RocMarshal reopened this Jul 27, 2026
@RocMarshal
RocMarshal force-pushed the ISSUE-65012 branch 2 times, most recently from c51e2d3 to 24c4218 Compare July 27, 2026 15:02
@rueian

rueian commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Hi @RocMarshal, please fix the lint.

@rueian rueian added the go add ONLY when ready to merge, run all tests label Jul 28, 2026
@rueian rueian self-assigned this Jul 28, 2026
@RocMarshal
RocMarshal force-pushed the ISSUE-65012 branch 2 times, most recently from 44f5eba to 093abbc Compare July 28, 2026 02:35
@RocMarshal

RocMarshal commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @rueian for the reminder.
Updated

@RocMarshal
RocMarshal force-pushed the ISSUE-65012 branch 3 times, most recently from 058624a to 4f025d7 Compare July 30, 2026 17:30
@RocMarshal
[core][raylet] Fix spill_manager_objects_bytes reports restored objec…
c9bb651 
…t count instead of restored bytes

Signed-off-by: Yuepeng Pan <panyuepeng@apache.org>
@rueian
rueian merged commit 158e505 into ray-project:master Jul 31, 2026
6 checks passed
@RocMarshal

RocMarshal commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @rueian ☕

Artimislyy pushed a commit to Artimislyy/ray that referenced this pull request Aug 11, 2026
@RocMarshal @artimislyy-netizen
[core][raylet] Fix spill_manager_objects_bytes reports restored objec…
18cf356 
…t count instead of restored bytes (ray-project#65013)

## Description

- What problem does this solve?

The `spill_manager_objects_bytes` metric appears to report the wrong
value for
`State="Restored"`.
In `LocalObjectManager::RecordMetrics()`, the restored sample for
`spill_manager_objects_bytes` is recorded with
`restored_objects_total_`:
                                                                 
```cpp
spill_manager_metrics_.spill_manager_objects_bytes_gauge.Record(
        restored_objects_total_, {{"State", "Restored"}});          
  ```
However, this metric is documented as a byte-size metric:
name = "spill_manager_objects_bytes"
  description = "Byte size of local objects broken per state ..." 



                                                               
- How does this PR solve it?

  correct the new value of the target metrics .

- Any relevant context for reviewers such as:

  ray-project#65012 

- Why is the problem important to solve?

  IIUC, It's a but for metrics on the corresponding value.

- Why was this approach chosen over others?

  direct & simple.

## Related issues

  Fixes ray-project#65012 

## Additional information
  N.A

Signed-off-by: Yuepeng Pan <panyuepeng@apache.org>
400Ping pushed a commit to 400Ping/ray that referenced this pull request Aug 18, 2026
@RocMarshal @400Ping
[core][raylet] Fix spill_manager_objects_bytes reports restored objec…
2fdda65 
…t count instead of restored bytes (ray-project#65013)

## Description

- What problem does this solve?

The `spill_manager_objects_bytes` metric appears to report the wrong
value for
`State="Restored"`.
In `LocalObjectManager::RecordMetrics()`, the restored sample for
`spill_manager_objects_bytes` is recorded with
`restored_objects_total_`:

```cpp
spill_manager_metrics_.spill_manager_objects_bytes_gauge.Record(
        restored_objects_total_, {{"State", "Restored"}});
  ```
However, this metric is documented as a byte-size metric:
name = "spill_manager_objects_bytes"
  description = "Byte size of local objects broken per state ..."

- How does this PR solve it?

  correct the new value of the target metrics .

- Any relevant context for reviewers such as:

  ray-project#65012

- Why is the problem important to solve?

  IIUC, It's a but for metrics on the corresponding value.

- Why was this approach chosen over others?

  direct & simple.

## Related issues

  Fixes ray-project#65012

## Additional information
  N.A

Signed-off-by: Yuepeng Pan <panyuepeng@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
nadongjun pushed a commit to nadongjun/ray that referenced this pull request Oct 6, 2026
@RocMarshal
[core][raylet] Fix spill_manager_objects_bytes reports restored objec…
d5fd7c5 
…t count instead of restored bytes (ray-project#65013)

## Description

- What problem does this solve?

The `spill_manager_objects_bytes` metric appears to report the wrong
value for
`State="Restored"`.
In `LocalObjectManager::RecordMetrics()`, the restored sample for
`spill_manager_objects_bytes` is recorded with
`restored_objects_total_`:
                                                                 
```cpp
spill_manager_metrics_.spill_manager_objects_bytes_gauge.Record(
        restored_objects_total_, {{"State", "Restored"}});          
  ```
However, this metric is documented as a byte-size metric:
name = "spill_manager_objects_bytes"
  description = "Byte size of local objects broken per state ..." 



                                                               
- How does this PR solve it?

  correct the new value of the target metrics .

- Any relevant context for reviewers such as:

  ray-project#65012 

- Why is the problem important to solve?

  IIUC, It's a but for metrics on the corresponding value.

- Why was this approach chosen over others?

  direct & simple.

## Related issues

  Fixes ray-project#65012 

## Additional information
  N.A

Signed-off-by: Yuepeng Pan <panyuepeng@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cursor cursor[bot] cursor[bot] left review comments
@rueian rueian rueian approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@rueian rueian

Labels

community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[core][raylet] spill_manager_objects_bytes reports restored object count instead of restored bytes

2 participants

@RocMarshal @rueian