You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "waitinfuture (via GitHub)" <gi...@apache.org> on 2024/03/07 12:50:01 UTC
[PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
waitinfuture opened a new pull request, #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
As title, `handleRequestPartitions` is quite heavy since it calls sync RPC.
It's unnecessary to put it in the sync block.
### Why are the changes needed?
ditto
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
GA and manual test.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#discussion_r1517052413
##########
client/src/main/scala/org/apache/celeborn/client/ChangePartitionManager.scala:
##########
@@ -19,7 +19,7 @@ package org.apache.celeborn.client
import java.util
import java.util.{Set => JSet}
-import java.util.concurrent.{ConcurrentHashMap, ScheduledExecutorService, ScheduledFuture, TimeUnit}
+import java.util.concurrent.{ConcurrentHashMap, ExecutorService, ScheduledExecutorService, ScheduledFuture, TimeUnit}
Review Comment:
+1
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#discussion_r1517330610
##########
client/src/main/scala/org/apache/celeborn/client/ChangePartitionManager.scala:
##########
@@ -19,7 +19,7 @@ package org.apache.celeborn.client
import java.util
import java.util.{Set => JSet}
-import java.util.concurrent.{ConcurrentHashMap, ScheduledExecutorService, ScheduledFuture, TimeUnit}
+import java.util.concurrent.{ConcurrentHashMap, ExecutorService, ScheduledExecutorService, ScheduledFuture, TimeUnit}
Review Comment:
fixed, thanks
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#issuecomment-1985189288
Thanks, merging to main(v0.5.0)/branch-0.4(v0.4.1)
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#discussion_r1516545476
##########
client/src/main/scala/org/apache/celeborn/client/ChangePartitionManager.scala:
##########
@@ -19,7 +19,7 @@ package org.apache.celeborn.client
import java.util
import java.util.{Set => JSet}
-import java.util.concurrent.{ConcurrentHashMap, ScheduledExecutorService, ScheduledFuture, TimeUnit}
+import java.util.concurrent.{ConcurrentHashMap, ExecutorService, ScheduledExecutorService, ScheduledFuture, TimeUnit}
Review Comment:
nit: Not required ?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "CodingCat (via GitHub)" <gi...@apache.org>.
CodingCat commented on PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#issuecomment-1983966838
+1, I am exactly looking at this yesterday
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2364: [CELEBORN-1312] Move handleRequestPartitions out of sync block
URL: https://github.com/apache/incubator-celeborn/pull/2364
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#issuecomment-1983466133
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2364?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 48.97%. Comparing base [(`835437f`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/835437f0b9d89e5c3ce44f5aacad2a7d396da1a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`7f9639c`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2364 +/- ##
==========================================
+ Coverage 48.83% 48.97% +0.14%
==========================================
Files 208 208
Lines 12984 12984
Branches 1115 1115
==========================================
+ Hits 6340 6358 +18
+ Misses 6233 6219 -14
+ Partials 411 407 -4
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2364?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1312] Move handleRequestPartitions out of sync block [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2364:
URL: https://github.com/apache/incubator-celeborn/pull/2364#issuecomment-1983443192
@lyy-pineapple PTAL, cc @RexXiong @AngersZhuuuu
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org