You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "zhouyifan279 (via GitHub)" <gi...@apache.org> on 2023/08/29 08:04:30 UTC
[GitHub] [incubator-celeborn] zhouyifan279 opened a new pull request, #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
zhouyifan279 opened a new pull request, #1853:
URL: https://github.com/apache/incubator-celeborn/pull/1853
…ion index args in ShuffleReader related methods
### What changes were proposed in this pull request?
Unify the order of map index args and partition index args in ShuffleReader related methods.
### Why are the changes needed?
The order of map index args and partition index args in CelebornShuffleReader constructor is different the order in `SparkShuffleManager#getReader`.
It can messed up easily.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Run tests locally.
--
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
[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1853:
URL: https://github.com/apache/incubator-celeborn/pull/1853#discussion_r1308851622
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
😭😭😭 Which api does Gluten celeborn shuffle manager rely on? It ok if we consistently make start/end partitions before map indices?
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
😭😭😭 Which api does Gluten celeborn shuffle manager rely on? It ok if we consistently make start/end partitions before map indices? cc @kerwin-zk
--
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
[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1853:
URL: https://github.com/apache/incubator-celeborn/pull/1853#discussion_r1308851622
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
😭😭😭
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
😭😭😭
--
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
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1853:
URL: https://github.com/apache/incubator-celeborn/pull/1853#issuecomment-1696977115
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1853](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (aeb9aae) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/7ab8c58aa755fb2a69906ec31d154ea52e806979?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7ab8c58) will **increase** coverage by `0.02%`.
> Report is 1 commits behind head on main.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1853 +/- ##
==========================================
+ Coverage 46.52% 46.54% +0.02%
==========================================
Files 163 163
Lines 10153 10153
Branches 934 934
==========================================
+ Hits 4723 4725 +2
+ Misses 5117 5116 -1
+ Partials 313 312 -1
```
[see 1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1853/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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
[GitHub] [incubator-celeborn] ulysses-you commented on a diff in pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #1853:
URL: https://github.com/apache/incubator-celeborn/pull/1853#discussion_r1308707701
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
It would break gluten celeborn shuffle manager...
--
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
[GitHub] [incubator-celeborn] waitinfuture closed pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
URL: https://github.com/apache/incubator-celeborn/pull/1853
--
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
[GitHub] [incubator-celeborn] zhouyifan279 commented on a diff in pull request #1853: [CELEBORN-919][FOLLOWUP] Unify the order of map index args and partit…
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #1853:
URL: https://github.com/apache/incubator-celeborn/pull/1853#discussion_r1310341278
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
Make another PR #1869 to fix 😂
##########
client-spark/spark-3/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornShuffleReader.scala:
##########
@@ -30,10 +30,10 @@ import org.apache.celeborn.common.CelebornConf
class CelebornShuffleReader[K, C](
handle: CelebornShuffleHandle[K, _, C],
- startPartition: Int,
- endPartition: Int,
startMapIndex: Int = 0,
endMapIndex: Int = Int.MaxValue,
+ startPartition: Int,
+ endPartition: Int,
Review Comment:
Make another PR #1869 to fix 😂
--
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