Skip to content

Conversation

@ferenc-csaky
Copy link
Contributor

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

@flinkbot
Copy link
Collaborator

flinkbot commented Dec 10, 2025

CI report:

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

Copy link
Contributor

@RocMarshal RocMarshal left a 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

@github-actions github-actions bot added the community-reviewed PR has been reviewed by the community. label Dec 10, 2025
@ferenc-csaky
Copy link
Contributor Author

ferenc-csaky commented Dec 15, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-reviewed PR has been reviewed by the community.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants