Skip to content

[CELEBORN-1917][FOLLOWUP] Do not wait if total inflight or inflight to a specific worker is lower than threshold#3386

Closed
turboFei wants to merge 1 commit into
apache:mainfrom
turboFei:1917_follow
Closed

[CELEBORN-1917][FOLLOWUP] Do not wait if total inflight or inflight to a specific worker is lower than threshold#3386
turboFei wants to merge 1 commit into
apache:mainfrom
turboFei:1917_follow

Conversation

@turboFei

@turboFei turboFei commented Jul 24, 2025

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

If maxInFlightBytesSizeEnabled, break the limitMaxInFlight waiting if total inflight or inflight to a specific worker is lower than threshold.

Why are the changes needed?

Followup for comment #3248 (comment) incase we miss it.

My earlier query was, should this be || ?
As in, either total inflight is high, or inflight to a specific worker is high (assuming per worker threshold is lower than total !)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Code review.

@turboFei turboFei changed the title Followup comments [CELEBORN-1917][FOLLOWUP] Jul 24, 2025
@turboFei turboFei changed the title [CELEBORN-1917][FOLLOWUP] [CELEBORN-1917][FOLLOWUP] Break if total inflight or inflight to a specific worker is lower than threshold Jul 24, 2025
@turboFei turboFei changed the title [CELEBORN-1917][FOLLOWUP] Break if total inflight or inflight to a specific worker is lower than threshold [CELEBORN-1917][FOLLOWUP] Do not wait if total inflight or inflight to a specific worker is lower than threshold Jul 24, 2025
@turboFei

Copy link
Copy Markdown
Member Author

cc @DDDominik @mridulm @FMX @RexXiong

@DDDominik

DDDominik commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

Using || could lead to global byte limit violations or single worker overload.
The logic here is not about 'when to limit the flow', but about 'when to stop waiting'. It can exit the waiting state only if neither limit is exceeded.

@mridulm mridulm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mridulm mridulm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, @DDDominik is right - I got the logic wrong.
What exists is correct - we dont need this: thanks for the follow up @turboFei - we can close this; apologies for the bother !

@turboFei turboFei closed this Jul 24, 2025
@turboFei

Copy link
Copy Markdown
Member Author

Actually, @DDDominik is right - I got the logic wrong.

What exists is correct - we dont need this: thanks for the follow up @turboFei - we can close this; apologies for the bother !

ok, closed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants