[FLINK-38717][runtime] Display the load status of TaskManagers.

[RocMarshal]

What is the purpose of the change

• [FLINK-38718][runtime] Display the number of tasks at slots level on the detailed TaskManager page.
• [FLINK-38719][runtime] Display the number of tasks on the TaskManagers page.

Brief change log

• [FLINK-38718][runtime] Display the number of tasks at slots level on the detailed TaskManager page.
• [FLINK-38719][runtime] Display the number of tasks on the TaskManagers page.

Verifying this change

This change added tests and can be verified as follows:

• org.apache.flink.runtime.resourcemanager.ResourceManagerTaskExecutorTest#testRequestTaskManagerDetailsInfo

• integration test result:

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)

[RocMarshal]

Hi, @ferenc-csaky Could you help take a look ? Thank you !

[flinkbot]

CI report:

• efbbb4e Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

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

[ferenc-csaky]

I think assignedTasks instead of numberOfTasks would cover better what this field is intended for.

I do not see why do we need to use LoadingWeight for this. It feels forced to me, and I maybe I miss something, but I also feel it complicates this change for nothing.

[RocMarshal]

I think assignedTasks instead of numberOfTasks would cover better what this field is intended for.

I do not see why do we need to use LoadingWeight for this. It feels forced to me, and I maybe I miss something, but I also feel it complicates this change for nothing.

Thank you @ferenc-csaky for the comments.

Pls let me have a try on clarifying it:

Initially, I did not use LoadingWeight when extending the SlotReport, and only included the numberOfTasks.

Why I switched to LoadingWeight:

• It satisfies the current functional requirements, too.
• One of the motivations behind introducing LoadingWeight is to provide a way to represent and potentially extend more slot-level load information.What I mean is: if we use LoadingWeight as the vehicle for reporting SlotReport data, then in the future, if we want to expose additional load-related information, we won’t need to change the load-related parameter passing chain anymore. The ResourceManager could simply retrieve the desired load metrics directly from LoadingWeight. In short, using an interface-driven approach gives us openness for future extensions. That said, I admit that, relative to the current functionality, this coding approach does violate the principle of minimal changes.

Both approaches are acceptable to me. If you believe that, for the current change, adhering to the minimal change principle should take priority, I'd be happy to revise the implementation accordingly.

[RocMarshal]

Hi @ferenc-csaky May I know your next suggestions or opinion if you had the free time ?
Thank you very much !

[ferenc-csaky]

@RocMarshal thanks for the detailed explanation. I was not aware what LoadingWeight do, but now I educated myself as well. I do not have problems applying it here, but I don't think its name good at all. To address that, I raised #27333, PTAL. I also added an int convenience method to avoid the casting everywhere you want to get its value.

[RocMarshal]

Thanks @ferenc-csaky for the review
Could you help take a look if you had the free time?

[ferenc-csaky]

Any specific reason to make this a nullable object instead of a primitive int, just like the other numeric statistics we already have? And I mean it for any interface/class we added this, so SlotManager and TaskManagerSlotInformation as well.

getNumberRegisteredSlotsOf(InstanceID instanceId) is also optional, and we just return 0 in that case. That's simpler code wise and in this context null means 0, so it seems to be fine, hence I'm not sure why we handle this differently.

[RocMarshal]

Thanks for the comments.

Since we currently synchronize the value of assignedTasks via slot reports, there may be an intermediate state where a null value is needed to represent it: a slot has not received any slot report information yet. In this case, representing assignedTasks as an uninitialized state (i.e., null) might be more appropriate.

I also agree with your point. This is really just a matter of choice, since in principle I likewise prefer the logic and implementation to be as simple as possible.

Updated.

[ferenc-csaky]

LGTM, thanks for iterating on this! CI failure seems orthogonal to this change, but I only gave it a quick look, I'll give you the chance to take a look and then we can merge if you agree as well.

[RocMarshal]

CI failure seems orthogonal to this change, but I only gave it a quick look, I'll give you the chance to take a look and then we can merge if you agree as well.

Thank you @ferenc-csaky for your hard review & checking.

+1, the ci was failed due to the flakey test cases likes [1][2], which is orthogonal to this change.
And I drive a no-edit commit to re-trigger a ci run by efbbb4e

[1] https://issues.apache.org/jira/browse/FLINK-38727
[2] https://issues.apache.org/jira/browse/FLINK-38843

[RocMarshal]

Thanks @ferenc-csaky for the idea to generate the patch and hard work on review 👍