[FLINK-38799][runtime] Rename LoadingWeight to TaskExecutionLoad

[ferenc-csaky]

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 TaskExecutionLoad
• Renamed WeightLoadable to HasTaskExecutionLoad
• Renamed DefaultLoadingWeight to DefaultTaskExecutionLoad
• Added getLoadValueAsInt() 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

[flinkbot]

CI report:

• 7530cb4 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[RocMarshal]

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

[ferenc-csaky]

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

[RocMarshal]

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.

[zhuzhurk]

Agreed that LoadingWeight is a bit too abstract.

From my understanding, the current LoadingWeight represents the load of task execution. At the moment, it is the number of tasks that will run in the given slot. Please correct me if I’m mistaken, @RocMarshal .
To me, SchedulingLoad sounds more like it refers to the overhead of scheduling tasks, rather than the actual execution load. So perhaps TaskExecutionLoad would be a clearer and more precise name.
More importantly, I think we should add more detailed comments to this class, so that one does not need to go through all the related code to find what it actually means.

What do you think? @ferenc-csaky

[ferenc-csaky]

@zhuzhurk @RocMarshal Renamed to TaskExecutionLoad. And now I did that I start wondering if TaskExecutorLoad would be better or not. There were variables named like that before. But I pretty much unified all the naming everywhere, so PTAL when you have some time.

[RocMarshal]

Thx @ferenc-csaky for the update.
It seems that some code check style issues occurred here(from CI log).

Would you mind having a check?

[ferenc-csaky]

Ofc I forgot to rerun it after I added the javadoc. Done now.

[zhuzhurk]

LGTM.

[RocMarshal]

Thanks @ferenc-csaky for the hard work and @zhuzhurk for the review.

LGTM +1.

Merging...