feat: move max_executors to limits.#396
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the cluster configuration schema by moving max_executors from the executors block to a centralized limits block and renaming the sessions field to max_sessions. These changes are reflected across configuration files, internal data structures, and tests. Review feedback suggests maintaining backward compatibility for the renamed field using serde aliases and cleaning up the now-obsolete FlameExecutorLimitsYaml structure.
| pub max_sessions: Option<usize>, | ||
| pub max_executors: Option<u32>, |
There was a problem hiding this comment.
The field sessions has been renamed to max_sessions. To maintain backward compatibility with existing configuration files, consider adding a serde alias so that the old field name is still recognized during deserialization.
#[serde(alias = "sessions")]
pub max_sessions: Option<usize>,
pub max_executors: Option<u32>,| fn try_from(executors: FlameExecutorsYaml) -> Result<Self, Self::Error> { | ||
| Ok(FlameExecutors { | ||
| shim: Shim::try_from(executors.shim.unwrap_or(DEFAULT_SHIM.to_string()))?, | ||
| limits: executors | ||
| .limits | ||
| .map(FlameExecutorLimits::try_from) | ||
| .unwrap_or_else(|| Ok(FlameExecutorLimits::default()))?, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<FlameExecutorLimitsYaml> for FlameExecutorLimits { | ||
| type Error = FlameError; | ||
| fn try_from(limits: FlameExecutorLimitsYaml) -> Result<Self, Self::Error> { | ||
| Ok(FlameExecutorLimits { | ||
| max_executors: limits | ||
| .max_executors | ||
| .unwrap_or(DEFAULT_MAX_EXECUTORS_PER_NODE), | ||
| }) | ||
| } |
There was a problem hiding this comment.
With max_executors moved to the limits block, the limits field in FlameExecutorsYaml and the FlameExecutorLimitsYaml struct are no longer used in the TryFrom implementation. These should be removed from the FlameExecutorsYaml definition to clean up the schema and avoid confusion.
1 participant
No description provided.