-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-38799][runtime] Rename LoadingWeight to ResourceUnitCount
#27333
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
base: master
Are you sure you want to change the base?
Conversation
RocMarshal
left a comment
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.
Thank you very much for your help and review on pre-pr, including the research work conducted before raising the current ticket.
It's tough to me to assess whether the current ticket is reasonable in my limited reading.
Perhaps I could provide some background on the history or rationale behind introducing the LoadingWeight abstraction:
Initially, LoadingWeight was designed to describe the load status of arbitrary abstract information on certain resource requests (such as ExecutionSlotSharingGroup in the AdaptiveScheduler and PendingRequest in the default scheduler) and resource entities (e.g., TaskManagers) during the scheduling process. This status could represent the number of tasks [1], the number of operators, or even declarative static resource unit values and counts (as described in the current ticket, though there is no prior precedent for this usage).
Therefore:
- If we proceed with the changes suggested in the current ticket, LoadingWeight would lose or see a reduction in its original scope of abstract representation.
- In scheduling-related logic, the number of tasks in certain resource requests and resource entities (as implemented in DefaultLoadingWeight) does not seem to align clearly with the definition of Resource Unit Count.
That said, I believe this ticket still warrants discussion or further progress. While we cannot be certain if a better interface or descriptor truly exists to replace LoadingWeight, we still hope to find and identify a more optimal representation.
Regarding the issues you mentioned earlier in [2]:
- Type casting issue:
Would it be possible to introduce a method like getAssignedOrContainedTasksdirectly in LoadingWeight? This could allow calling the method directly and avoid the need for type casting. - Current PR review process:
This PR and the merge order of [2] should not have a strict dependency and can likely proceed in parallel.
I'm trying to speculate whether you simply find the usage of LoadingWeight in [2] inappropriate. If that's the case, then adopting a new set of interface abstractions to handle parameter passing and corresponding logic calculations throughout the entire chain—without altering how LoadingWeight is used in [1]—sounds like an excellent alternative.
Please correct me if I'm wrong with the points mentioned above.
Thank you~
[1] https://issues.apache.org/jira/browse/FLINK-31757
[2] #27291
|
@RocMarshal Thanks a lot for taking the time and effort to summarize where the original implementation comes from. Based on that, I agree that using the name However, my main problem here is with the name itself. So based on this, I'd suggest the naming below:
I'm open to other ideas, but I do not think we should leave it as is.
I'm not sure I get the intent completely, but I really believe we should not make this more complex for now (I really like the "you ain't gonna need it" principle, and the best code is the code that does not exist, cause that won't have bugs). |
What is the purpose of the change
Rename
LoadingWeightand its accompanying class, interface to be more intuitive what it represents.Brief change log
LoadingWeighttoResourceUnitCountWeightLoadabletoHasResourceUnitDefaultLoadingWeighttoDefaultResourceUnitCountgetCountAsInt()for convenienceVerifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation