[FLINK-38799][runtime] Rename `LoadingWeight` to `ResourceUnitCount`
What is the purpose of the change
Rename LoadingWeight and its accompanying class, interface to be more intuitive what it represents.
Brief change log
- Renamed
LoadingWeighttoResourceUnitCount - Renamed
WeightLoadabletoHasResourceUnit - Renamed
DefaultLoadingWeighttoDefaultResourceUnitCount - Added
getCountAsInt()for convenience - Covered the new method with unit tests
Verifying 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:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
@Public(Evolving): no - The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
CI report:
- 60810690e72618af6e0c94f28f9a4b8701399d18 Azure: SUCCESS
Bot commands
The @flinkbot bot supports the following commands:@flinkbot run azurere-run the last Azure build
@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 ResourceUnitCount would be wrong if we take the original intent into account.
However, my main problem here is with the name itself. LoadingWeight is too abstract, means and tied to pretty much nothing, so if a couple months from now somebody else are in the same shoes as me, they still need to spend significant amount of time to understand what's going on here actually. WeightLoadable IMO is a bit worse, cause it's a) grammatically does not represent what the interface can be used to, and b) not obvious it is connected to LoadingWeight (if I just bump into it in the code where it is applied, if i check the package structure that clarifies it ofc).
So based on this, I'd suggest the naming below:
LoadingWeight->SchedulingLoad(with agetLoad()method)WeightLoadable->HasSchedulingLoad(withgetSchedulingLoad())
I'm open to other ideas, but I do not think we should leave it as is.
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.
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).
Thanks @ferenc-csaky
- About the interfaces changing in the current PR.
So based on this, I'd suggest the naming below:
LoadingWeight->SchedulingLoad(with agetLoad()method)WeightLoadable->HasSchedulingLoad(withgetSchedulingLoad())
Could you @zhuzhurk @1996fanrui help take a look ? thank a lot.
- About change in other PR need to do:
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).
@ferenc-csaky Thanks for the clarification. I'll make the change ASAP.