Skip to content

bugfix: support per-fork disagg pd port for fork master.#1220

Open
Clement-Wang26 wants to merge 1 commit intojd-opensource:mainfrom
Clement-Wang26:xtensor_pd
Open

bugfix: support per-fork disagg pd port for fork master.#1220
Clement-Wang26 wants to merge 1 commit intojd-opensource:mainfrom
Clement-Wang26:xtensor_pd

Conversation

@Clement-Wang26
Copy link
Copy Markdown
Collaborator

No description provided.

@Clement-Wang26 Clement-Wang26 changed the title bugfix: support per-fork disagg_pd_port for fork_master. bugfix: support per-fork disagg pd port for fork master. Apr 8, 2026
Copy link
Copy Markdown
Contributor

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

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 introduces an optional disagg_pd_port configuration across the API service, master instances, and schedulers, enabling more flexible port assignment during master forking. The changes include updates to the protobuf definitions, options handling, and RPC server startup logic to propagate this port. Technical feedback identifies a likely compilation error in the logging code within disagg_pd_scheduler.cpp and pd_ooc_scheduler.cpp, where to_string() is called on an InstanceRole enum, which typically does not possess such a member method in C++.

Comment on lines +271 to +273
<< (options_.instance_role().has_value()
? options_.instance_role().value().to_string()
: "unknown")
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.

high

The instance_role() method in ContinuousScheduler::Options returns an std::optional<InstanceRole>. While the ternary operator correctly checks has_value(), the InstanceRole type itself (likely an enum class based on the style guide) does not have a to_string() member method. This will cause a compilation error. You should use a helper function or a global to_string overload if available, or ensure InstanceRole is a class that provides this method.

Comment on lines +144 to +146
<< (options_.instance_role().has_value()
? options_.instance_role().value().to_string()
: "unknown")
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.

high

The instance_role() method in ContinuousScheduler::Options returns an std::optional<InstanceRole>. While the ternary operator correctly checks has_value(), the InstanceRole type itself (likely an enum class based on the style guide) does not have a to_string() member method. This will cause a compilation error. You should use a helper function or a global to_string overload if available, or ensure InstanceRole is a class that provides this method.

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.

1 participant