Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CELEBORN-1725][FOLLOWUP] Optimize isAllMapTasksEnd performance #2959

Closed
wants to merge 4 commits into from
Closed

[CELEBORN-1725][FOLLOWUP] Optimize isAllMapTasksEnd performance #2959

wants to merge 4 commits into from
+45 −16

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Nov 27, 2024 •
edited
Loading

What changes were proposed in this pull request?

Followup for #2905,

using the same logic to optimize isAllMapTasksEnd method.

Why are the changes needed?

Address comments: #2905 (review)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Same logic with #2905

onebox-li
onebox-li reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@onebox-li onebox-li left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! I think this code fragment could be polished too . https://github.com/apache/celeborn/blob/main/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala#L455-L466

@FMX
Copy link
Contributor

FMX commented Nov 28, 2024

You can add a new PR to fix all similar code snippets. There are still some more exists codes.
For example, there are similar codes in ReducePartitionCommitHandler.

@turboFei turboFei marked this pull request as draft November 28, 2024 04:24
@turboFei turboFei marked this pull request as ready for review November 28, 2024 04:25
@turboFei
Copy link
Member Author

You can add a new PR to fix all similar code snippets. There are still some more exists codes. For example, there are similar codes in ReducePartitionCommitHandler.

Ok, thx

@turboFei
comments
d9012d1 
utils
@turboFei
Copy link
Member Author

turboFei commented Nov 28, 2024 •
edited
Loading

Thanks! I think this code fragment could be polished too . https://github.com/apache/celeborn/blob/main/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala#L455-L466

Thanks for the suggestion, will address it in CELEBORN-1753 for exists and find

@turboFei turboFei mentioned this pull request Nov 28, 2024
Draft
@RexXiong RexXiong closed this in c84733f Nov 29, 2024
RexXiong pushed a commit that referenced this pull request Nov 29, 2024
@turboFei @RexXiong
[CELEBORN-1725][FOLLOWUP] Optimize isAllMapTasksEnd performance
d97d58f 
### What changes were proposed in this pull request?

Followup for #2905,

using the same logic to optimize `isAllMapTasksEnd` method.

### Why are the changes needed?
Address comments: #2905 (review)

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
Same logic with #2905

Closes #2959 from turboFei/celeborn_1725_follow.

Authored-by: Wang, Fei <[email protected]>
Signed-off-by: Shuang <[email protected]>
(cherry picked from commit c84733f)
Signed-off-by: Shuang <[email protected]>
RexXiong pushed a commit that referenced this pull request Nov 29, 2024
@turboFei @RexXiong
[CELEBORN-1725][FOLLOWUP] Optimize isAllMapTasksEnd performance
93bdda5 
Followup for #2905,

using the same logic to optimize `isAllMapTasksEnd` method.

Address comments: #2905 (review)

No.

Same logic with #2905

Closes #2959 from turboFei/celeborn_1725_follow.

Authored-by: Wang, Fei <[email protected]>
Signed-off-by: Shuang <[email protected]>
(cherry picked from commit c84733f)
Signed-off-by: Shuang <[email protected]>
@RexXiong
Copy link
Contributor

Thanks, merge to main(V0.6.0) and branch-0.5(v0.5.3) and branch-0.4(v0.4.3)

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

@RexXiong RexXiong RexXiong approved these changes

@pan3793 pan3793 pan3793 approved these changes

@onebox-li onebox-li onebox-li approved these changes

@cfmcgrady cfmcgrady Awaiting requested review from cfmcgrady

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@turboFei @FMX @RexXiong @onebox-li @pan3793