[core] Add comments explaining the usage of the `ray_syncer_` channels in the Raylet #58342

edoakes · 2025-10-31T14:58:52Z

Found it very hard to parse what was happening here, so helping future me (or you!).

Also:

Deleted vestigial next_resource_seq_no_.
Converted from non-monotonic clock to a monotonically incremented uint64_t for the version number for commands.
Added logs when we drop messages with stale versions.

edoakes · 2025-10-31T14:59:03Z

src/ray/raylet/node_manager.cc

      cluster_lease_manager_(cluster_lease_manager),
      record_metrics_period_ms_(config.record_metrics_period_ms),
      placement_group_resource_manager_(placement_group_resource_manager),
-      next_resource_seq_no_(0),


this was vestigial

what does vestigial mean lol

gemini-code-assist

Code Review

This pull request improves the clarity of the ray_syncer usage in the NodeManager by adding detailed comments explaining the RESOURCE_VIEW and COMMANDS channels. Additionally, it refactors the versioning for COMMANDS channel messages, replacing a time-based version with a monotonic counter, which is a more robust approach. However, this change introduces a compilation error because a const method attempts to modify a member variable. I've left a comment with a suggested fix.

src/ray/raylet/node_manager.h

Signed-off-by: Edward Oakes <[email protected]>

edoakes · 2025-10-31T15:10:09Z

src/ray/ray_syncer/ray_syncer_bidi_reactor_base.h

-          std::move(message);
-      StartSend();
-      return true;
+    if (node_versions[message->message_type()] >= message->version()) {


no behavior change, just reversed the early return logic

Sparks0219 · 2025-11-04T00:20:37Z

src/ray/raylet/node_manager.cc

-    // Register resource manager and scheduler
+    // RESOURCE_VIEW is used to synchronize available resources across Raylets.
+    //
+    // LocalResourceManager::CreateSyncMessage will be called periodically to collect


I think it's good to mention that it's both periodically called and also on-demand when local resources change.

Actually, it is not called on-demand! Inside of OnResourceOrStateChanged, we increment the version but we do not actually eagerly broadcast.

Oh whattt, thanks for the clarification 🤯

src/ray/raylet/node_manager.cc

src/ray/raylet/node_manager.h

Sparks0219 · 2025-11-04T01:07:01Z

src/ray/raylet/node_manager.cc

          if (triggered_by_global_gc) {
+            // Always increment the sync message version number so that all GC commands
+            // are sent indiscriminately.
+            gc_command_sync_version_++;


I think it's good to mention that even though we call OnDemandBroadcasting, it's only sent to the GCS and not to other raylets

I think it would also be good to mention in BroadcastMessage or the map of sync_reactors_ that for node managers, we only have one bidi reactor which is to the GCS. GCS has multiple bidi reactors, one for each node. Hence just to emphasize that it's NOT all to all on the raylet level, it's node to GCS to all nodes

I included this where we initialize the syncer and in the ray_syncer_ field comment. Putting it here specifically felt odd because it applies to all usage of the syncer.

Sparks0219

Think the comments could be a bit more refined

Sparks0219 · 2025-11-07T20:44:17Z

src/ray/raylet/node_manager.cc

  }
 }

 std::optional<syncer::RaySyncMessage> NodeManager::CreateSyncMessage(


nit: Can we rename this to CreateSyncCommandsMessage?

we cannot because it's a virtual method to implement the sync broadcaster interface. I tried to do that already :'(

ohh... I see both LocalResourceManager and NodeManager inherit from the syncer class and override this RIP

Sparks0219

LGTM 🚢

…s in the Raylet (ray-project#58342) Found it very hard to parse what was happening here, so helping future me (or you!). Also: - Deleted vestigial `next_resource_seq_no_`. - Converted from non-monotonic clock to a monotonically incremented `uint64_t` for the version number for commands. - Added logs when we drop messages with stale versions. --------- Signed-off-by: Edward Oakes <[email protected]>

…s in the Raylet (ray-project#58342) Found it very hard to parse what was happening here, so helping future me (or you!). Also: - Deleted vestigial `next_resource_seq_no_`. - Converted from non-monotonic clock to a monotonically incremented `uint64_t` for the version number for commands. - Added logs when we drop messages with stale versions. --------- Signed-off-by: Edward Oakes <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>

…s in the Raylet (ray-project#58342) Found it very hard to parse what was happening here, so helping future me (or you!). Also: - Deleted vestigial `next_resource_seq_no_`. - Converted from non-monotonic clock to a monotonically incremented `uint64_t` for the version number for commands. - Added logs when we drop messages with stale versions. --------- Signed-off-by: Edward Oakes <[email protected]> Signed-off-by: YK <[email protected]>

…s in the Raylet (ray-project#58342) Found it very hard to parse what was happening here, so helping future me (or you!). Also: - Deleted vestigial `next_resource_seq_no_`. - Converted from non-monotonic clock to a monotonically incremented `uint64_t` for the version number for commands. - Added logs when we drop messages with stale versions. --------- Signed-off-by: Edward Oakes <[email protected]>

edoakes added the go add ONLY when ready to merge, run all tests label Oct 31, 2025

edoakes requested a review from a team as a code owner October 31, 2025 14:58

edoakes commented Oct 31, 2025

View reviewed changes

edoakes force-pushed the eoakes/cleanup-syncer branch 2 times, most recently from e755861 to 3edc6de Compare October 31, 2025 15:00

gemini-code-assist bot reviewed Oct 31, 2025

View reviewed changes

src/ray/raylet/node_manager.h Show resolved Hide resolved

comments

1e41918

Signed-off-by: Edward Oakes <[email protected]>

edoakes force-pushed the eoakes/cleanup-syncer branch from 3edc6de to 1e41918 Compare October 31, 2025 15:03

edoakes added 2 commits October 31, 2025 10:07

fix

5b67c0f

Signed-off-by: Edward Oakes <[email protected]>

fix

4a0ef95

Signed-off-by: Edward Oakes <[email protected]>

edoakes commented Oct 31, 2025

View reviewed changes

edoakes assigned Sparks0219 Oct 31, 2025

ray-gardener bot added docs An issue or change related to documentation core Issues that should be addressed in Ray Core observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling labels Oct 31, 2025

edoakes enabled auto-merge (squash) November 3, 2025 13:19

Sparks0219 reviewed Nov 4, 2025

View reviewed changes

src/ray/raylet/node_manager.cc Show resolved Hide resolved

Sparks0219 reviewed Nov 4, 2025

View reviewed changes

src/ray/raylet/node_manager.cc Show resolved Hide resolved

Sparks0219 reviewed Nov 4, 2025

View reviewed changes

src/ray/raylet/node_manager.h Show resolved Hide resolved

Sparks0219 reviewed Nov 4, 2025

View reviewed changes

Sparks0219 requested changes Nov 4, 2025

View reviewed changes

edoakes requested a review from Sparks0219 November 4, 2025 02:49

Sparks0219 reviewed Nov 7, 2025

View reviewed changes

Sparks0219 approved these changes Nov 7, 2025

View reviewed changes

edoakes merged commit 97de782 into ray-project:master Nov 7, 2025
7 checks passed

[core] Add comments explaining the usage of the ray_syncer_ channels in the Raylet #58342

[core] Add comments explaining the usage of the ray_syncer_ channels in the Raylet #58342

Uh oh!

Conversation

edoakes commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks0219 left a comment

Choose a reason for hiding this comment

Uh oh!

Sparks0219 Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks0219 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[core] Add comments explaining the usage of the `ray_syncer_` channels in the Raylet #58342

[core] Add comments explaining the usage of the `ray_syncer_` channels in the Raylet #58342

edoakes commented Oct 31, 2025 •

edited

Loading

Sparks0219 Nov 7, 2025 •

edited

Loading