flink icon indicating copy to clipboard operation
flink copied to clipboard

[FLINK-38799][runtime] Rename `LoadingWeight` to `ResourceUnitCount`

Open ferenc-csaky opened this issue 1 week ago • 1 comments

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 LoadingWeight to ResourceUnitCount
  • Renamed WeightLoadable to HasResourceUnit
  • Renamed DefaultLoadingWeight to DefaultResourceUnitCount
  • 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

ferenc-csaky avatar Dec 10 '25 14:12 ferenc-csaky

CI report:

  • 60810690e72618af6e0c94f28f9a4b8701399d18 Azure: SUCCESS
Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

flinkbot avatar Dec 10 '25 14:12 flinkbot

@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 a getLoad() method)
  • WeightLoadable -> HasSchedulingLoad (with getSchedulingLoad())

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).

ferenc-csaky avatar Dec 15 '25 17:12 ferenc-csaky

Thanks @ferenc-csaky

  • About the interfaces changing in the current PR.

So based on this, I'd suggest the naming below:

  • LoadingWeight -> SchedulingLoad (with a getLoad() method)
  • WeightLoadable -> HasSchedulingLoad (with getSchedulingLoad())

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.

RocMarshal avatar Dec 21 '25 16:12 RocMarshal