Skip to content

Conversation

RocMarshal
Copy link
Contributor

What is the purpose of the change

[FLINK-33387][runtime] Introduce the abstraction and the interface about loading

Brief change log

[FLINK-33387][runtime] Introduce the abstraction and the interface about loading

Verifying this change

  • Added a test class DefaultLoadingWeightTest

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@flinkbot
Copy link
Collaborator

flinkbot commented Aug 18, 2025

CI report:

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

@RocMarshal RocMarshal marked this pull request as ready for review August 18, 2025 08:42
* abstractions.
*/
@Internal
public interface WeightLoadable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise this is an implementation for the design. I find the naming confusing - for me it reads that LoadingWeight and WeightLoadable are opposites in some sense. But I do not think that is what we mean as one contains the other.

I suggest WeightLoadable and WeightLoading would be more straight forward.

@davidradl
Copy link
Contributor

davidradl commented Aug 26, 2025

@RocMarshal as there is a big design associated with this - for me it seems like this change would warrant a Flip. I know this has been long running and there seems to be energy at the moment to get it implemented, which I think is probably more important. WDYT?

@RocMarshal
Copy link
Contributor Author

RocMarshal commented Aug 26, 2025

@davidradl Thanks for the review and comments.
This is just a very small part of the implementation for the ongoing FLIP-370. What I mean is that the Loading-related implementation details we’re mentioning here can be discussed on the PR page, since they don’t affect the design concepts or direction — it’s purely a naming issue.
We only need to update the corresponding names in the FLIP document after we finalize the descriptions of the abstractions to be changed.
Please let me know your thoughts.

@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Aug 26, 2025
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