-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Convert StageId and TaskId to a record #26795
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
|
Important because of https://github.com/openjdk/jdk/blob/dfd383224dbc2e41c9f44b1acd09ffb179cc38f3/src/hotspot/share/ci/ciField.cpp#L237 Also less lines :) |
93aee4c to
820e117
Compare
820e117 to
b2299e9
Compare
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public class TaskId | ||
| public record TaskId(String fullId, StageId stageId, int partitionId, int attemptId) |
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.
This now exposes a bunch of private fields, so it should not be a record.
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.
Except for fullId all of these fields were accessed through the corresponding getters:
public StageId getStageId()
{
return stageId;
}
public int getPartitionId()
{
return partitionId;
}
public int getAttemptId()
{
return attemptId;
}
I don't see how accessing fullId() which you can anyway get through toString() breaks encapsulation. All of these fields were accessible externally through the previously exposed API
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.
It does break encapsulation because it allows TaskId to be constructed with stageId/partitionId/attemptId that don't agree with fullId
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.
QueryId was already converted
Description
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: