-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[java] encapsulation + resource immutability for option classes #54370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7b4a057
to
607a3e5
Compare
Signed-off-by: axreldable <[email protected]>
607a3e5
to
14f7bd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors several option classes to enforce encapsulation and immutability, and updates code sites to use the new getters.
- Converted public fields in option classes to private finals with getters and builder patterns
- Wrapped mutable collections (resources, bundles, concurrency groups) in unmodifiable views
- Updated runtime/task submitter and core runtime code to replace field access with the new getters
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
java/runtime/.../NativeTaskSubmitter.java | Replaced direct field access with getters |
java/runtime/.../LocalModeTaskSubmitter.java | Replaced direct field access with getters |
java/runtime/.../AbstractRayRuntime.java | Switched to getters for jvmOptions |
java/api/.../PlacementGroupCreationOptions.java | Introduced builder pattern and immutable bundles |
java/api/.../CallOptions.java | Introduced builder pattern and getters |
java/api/.../BaseTaskOptions.java | Made resources map immutable with getter |
java/api/.../ActorCreationOptions.java | Introduced builder pattern and getters |
Comments suppressed due to low confidence (4)
java/api/src/main/java/io/ray/api/options/CallOptions.java:43
- [nitpick] The getter name getSerializedRuntimeEnvInfo differs from ActorCreationOptions' getSerializedRuntimeEnv; consider aligning these names for consistency.
public String getSerializedRuntimeEnvInfo() {
java/api/src/main/java/io/ray/api/options/ActorCreationOptions.java:87
- [nitpick] The getter name getSerializedRuntimeEnv is inconsistent with CallOptions' getSerializedRuntimeEnvInfo; unifying naming across option classes would improve API clarity.
public String getSerializedRuntimeEnv() {
java/api/src/main/java/io/ray/api/options/CallOptions.java:117
- Switching from a public constructor to a builder-only approach is a breaking change; consider deprecating the old constructor or providing migration notes in the API docs.
return new CallOptions(this);
java/runtime/src/main/java/io/ray/runtime/task/NativeTaskSubmitter.java:63
- [nitpick] The error message 'Actor of name %s exists' could be clearer; for example, 'An actor with name %s already exists.'
!actor.isPresent(), String.format("Actor of name %s exists", options.getName()));
Preconditions.checkArgument( | ||
options.bundleIndex == -1 | ||
|| options.bundleIndex >= 0 && options.bundleIndex < group.getBundles().size(), | ||
options.getBundleIndex() == -1 | ||
|| options.getBundleIndex() >= 0 | ||
&& options.getBundleIndex() < group.getBundles().size(), | ||
String.format( | ||
"Bundle index %s is invalid, the correct bundle index should be " | ||
+ "either in the range of 0 to the number of bundles " | ||
+ "or -1 which means put the task to any available bundles.", | ||
options.bundleIndex)); | ||
options.getBundleIndex())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider assigning options.getBundleIndex() to a local variable to avoid repeated method calls and improve readability.
Copilot uses AI. Check for mistakes.
Signed-off-by: axreldable <[email protected]>
Signed-off-by: axreldable <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axreldable Thanks for the contribution! This is in line with my expectation. Left one minor comment.
… BaseTaskOptions class + unit tests Signed-off-by: axreldable <[email protected]>
Signed-off-by: axreldable <[email protected]>
@kfstorm @SongGuyang @WangTaoTheTonic @raulchen Could you please take a look and merge the PR? |
Hi @kfstorm @SongGuyang @WangTaoTheTonic @raulchen , |
…project#54370) Signed-off-by: axreldable <[email protected]> Co-authored-by: Sagar Sumit <[email protected]> Signed-off-by: dshepelev15 <[email protected]>
…project#54370) Signed-off-by: axreldable <[email protected]> Co-authored-by: Sagar Sumit <[email protected]> Signed-off-by: alimaazamat <[email protected]>
…project#54370) Signed-off-by: axreldable <[email protected]> Co-authored-by: Sagar Sumit <[email protected]> Signed-off-by: Krishna Kalyan <[email protected]>
Why are these changes needed?
Add encapsulation of fields and immutability for exposed structures such as maps and lists in classes within the
io.ray.api.options
module, as requested in issue #54310.Related issue number
Closes #54310
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.