You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "ferenc-csaky (via GitHub)" <gi...@apache.org> on 2025/12/10 14:05:27 UTC
[PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `ResourceUnitCount` [flink]
ferenc-csaky opened a new pull request, #27333:
URL: https://github.com/apache/flink/pull/27333
## 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `ResourceUnitCount` [flink]
Posted by "ferenc-csaky (via GitHub)" <gi...@apache.org>.
ferenc-csaky commented on PR #27333:
URL: https://github.com/apache/flink/pull/27333#issuecomment-3656850517
@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 bum.
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `SchedulingLoad` [flink]
Posted by "ferenc-csaky (via GitHub)" <gi...@apache.org>.
ferenc-csaky commented on code in PR #27333:
URL: https://github.com/apache/flink/pull/27333#discussion_r2718595020
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/FlinkRelOptUtil.scala:
##########
@@ -191,9 +191,64 @@ object FlinkRelOptUtil {
* @return
* The digest of given rel tree.
*/
+ // --- FLINK CUSTOM START: CorrelationId normalization for subplan reuse
+ private def normalizeCorrelationIds(rel: RelNode): RelNode = {
Review Comment:
Good catch, seems I messed up my git stash, thanks for pointing it out!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `SchedulingLoad` [flink]
Posted by "ferenc-csaky (via GitHub)" <gi...@apache.org>.
ferenc-csaky commented on PR #27333:
URL: https://github.com/apache/flink/pull/27333#issuecomment-3773901986
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `ResourceUnitCount` [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #27333:
URL: https://github.com/apache/flink/pull/27333#issuecomment-3637279777
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "60810690e72618af6e0c94f28f9a4b8701399d18",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "60810690e72618af6e0c94f28f9a4b8701399d18",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 60810690e72618af6e0c94f28f9a4b8701399d18 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `ResourceUnitCount` [flink]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal commented on PR #27333:
URL: https://github.com/apache/flink/pull/27333#issuecomment-3679010263
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `SchedulingLoad` [flink]
Posted by "zhuzhurk (via GitHub)" <gi...@apache.org>.
zhuzhurk commented on PR #27333:
URL: https://github.com/apache/flink/pull/27333#issuecomment-3753138491
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `TaskExecutionLoad` [flink]
Posted by "ferenc-csaky (via GitHub)" <gi...@apache.org>.
ferenc-csaky commented on PR #27333:
URL: https://github.com/apache/flink/pull/27333#issuecomment-3788427240
Ofc I forgot to rerun it after I added the javadoc. Done now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `SchedulingLoad` [flink]
Posted by "zhuzhurk (via GitHub)" <gi...@apache.org>.
zhuzhurk commented on code in PR #27333:
URL: https://github.com/apache/flink/pull/27333#discussion_r2715753463
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/slotpool/TasksBalancedRequestSlotMatchingStrategy.java:
##########
@@ -130,32 +131,35 @@ public int comparePriority(PhysicalSlotElement left, PhysicalSlotElement right)
public Collection<RequestSlotMatch> matchRequestsAndSlots(
Collection<? extends PhysicalSlot> slots,
Collection<PendingRequest> pendingRequests,
- Map<ResourceID, LoadingWeight> taskExecutorsLoad) {
+ Map<ResourceID, TaskExecutionLoad> taskExecutionLoadMap) {
ResourceRequestPreMappings resourceRequestPreMappings =
ResourceRequestPreMappings.createFrom(pendingRequests, slots);
if (!resourceRequestPreMappings.isMatchingFulfilled()) {
return Collections.emptyList();
}
final Collection<RequestSlotMatch> resultingMatches = new ArrayList<>();
- final List<PendingRequest> sortedRequests = sortByLoadingDescend(pendingRequests);
+ final List<PendingRequest> sortedRequests = sortDesc(pendingRequests);
Review Comment:
Maybe `sortByTaskExecutionLoadDesc` to clarify the method's behavior?
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/FlinkRelOptUtil.scala:
##########
@@ -191,9 +191,64 @@ object FlinkRelOptUtil {
* @return
* The digest of given rel tree.
*/
+ // --- FLINK CUSTOM START: CorrelationId normalization for subplan reuse
+ private def normalizeCorrelationIds(rel: RelNode): RelNode = {
Review Comment:
The change seems to be unrelated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38799][runtime] Rename `LoadingWeight` to `TaskExecutionLoad` [flink]
Posted by "RocMarshal (via GitHub)" <gi...@apache.org>.
RocMarshal merged PR #27333:
URL: https://github.com/apache/flink/pull/27333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org