You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/03/06 07:31:54 UTC
[GitHub] [spark] hiboyang opened a new pull request #31763: Map status metadata2
hiboyang opened a new pull request #31763:
URL: https://github.com/apache/spark/pull/31763
This PR is copied from https://github.com/apache/spark/pull/30004, with extra change addressing the comments. My git environment was messed up and could not update previous PR 30004. Thus create this new PR to replace the previous one.
### What changes were proposed in this pull request?
Add generic metadata in MapStatus class to support custom shuffle manager. Also add a new method to retrieve all map output statuses and their metadata. See Jira: https://issues.apache.org/jira/projects/SPARK/issues/SPARK-33114
### Why are the changes needed?
Current MapStatus class is tightly bound with local (sort merge) shuffle which uses BlockManagerId to store the shuffle data location. It could not support other custom shuffle manager implementation.
For example, when we implement Remote Shuffle Service, we want to put remote shuffle server information into MapStatus so reducer could fetch that information and figure out where to fetch data. The added MapStatus.metadata field could store such information.
If people implement other shuffle manager, they could also store their related information into this metadata field.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added unit 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r590917443
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
[SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) is very big, and may take very long time to finish (already being there for 10 months). Could we merge this PR first?
If [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) find a better way to support it and break `getAllMapOutputStatusMetadata`, it is actually a good thing :) We could have multiple iterations. This PR is the first iteration with very small change. [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) is the iteration after that. The latter does not need to block the former one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791912711
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135825/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589060306
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
@Ngone51 I agree with you that finishing the design laid out in SPARK-25299 would be much better.
This is why I opened https://github.com/apache/spark/pull/30763 as a copy of Matthew Cheah's original PR for SPARK-31801 (because he is busy with other projects) and kept it up-to-date several times with the master.
But it haven't got enough reviews and I wouldn't want to block @hiboyang further, https://github.com/apache/spark/pull/30004#issuecomment-782915655.
I am sure with your help we can complete SPARK-31801 and be on the road of SPARK-25299.
So next week I will do the conflict resolution and ping you when the PR is ready for review. Is this okay?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792362195
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40428/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r591061966
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
I see, I missed the latest discussion in #30763, will check there as well, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589964037
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
Yes, in this case, agree with you that this PR is a different topic from [SPARK-25299](https://issues.apache.org/jira/browse/SPARK-25299), and the community not have enough bandwidth to work on these two significant projects concurrently. This PR is adding a simple change to make it possible that different custom shuffle managers could add their own metadata inside the MapStatus object. Could we proceed to review this PR then?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r588996392
##########
File path: core/src/test/scala/org/apache/spark/MapOutputTrackerSuite.scala
##########
@@ -146,13 +170,18 @@ class MapOutputTrackerSuite extends SparkFunSuite {
intercept[FetchFailedException] { mapWorkerTracker.getMapSizesByExecutorId(10, 0) }
val size1000 = MapStatus.decompressSize(MapStatus.compressSize(1000L))
- masterTracker.registerMapOutput(10, 0, MapStatus(
- BlockManagerId("a", "hostA", 1000), Array(1000L), 5))
+ val mapStatus = MapStatus(BlockManagerId("a", "hostA", 1000), Array(1000L), 5)
+ masterTracker.registerMapOutput(10, 0, mapStatus)
mapWorkerTracker.updateEpoch(masterTracker.getEpoch)
assert(mapWorkerTracker.getMapSizesByExecutorId(10, 0).toSeq ===
Seq((BlockManagerId("a", "hostA", 1000),
ArrayBuffer((ShuffleBlockId(10, 5, 0), size1000, 0)))))
- assert(0 == masterTracker.getNumCachedSerializedBroadcast)
Review comment:
Please do not remove this assert:
```
assert(0 == masterTracker.getNumCachedSerializedBroadcast)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589993348
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
I can see how this PR would work. But even if we merged this PR, the ongoing SPARK-31801 may override your change before the release. `getAllMapOutputStatusMetadata` isn't an API, so you may suffer the breaking change. We're discussing a general way in SPARK-31801 to provide the API for users. IIUC, it'd also benefit your case.
WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873893785
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-821022941
**[Test build #137478 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137478/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792381251
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135846/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792357397
**[Test build #135846 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135846/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791899103
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40407/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589188420
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
I think we should continue the discussion based on the [SPIP](https://docs.google.com/document/d/1d6egnL6WHOwWZe8MWv3m8n4PToNacdx7n_0iMSWwhCQ/edit#) of SPARK-25299. I assume the SPIP of SPARK-25299 already passed since there're already some changes merged in, which means people already discussed other solutions previously. But sure, feel free to raise your concern if the current design still can't cover your use case.
And we actually know what we need to do (you can also check the sub-tasks of SPARK-25299). By further discussion, I mean, for the case like SPARK-31801, 1) we could split it into smaller pieces since its current scope is too big (but we need to take a look at the PR first) 2) the current implementation is still not finalized, we can discuss other ways for SPARK-31801.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589166884
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
Yes, it will be great if we continue to discuss and make progress for #30763. Maybe we could have a sync up zoom meeting to discuss how to split the big work? There are several remote/disaggregated shuffle solutions from Spark community and different companies. We could walk through these solutions and come out ideas about the minimum set of changes to support them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-821101839
**[Test build #137478 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137478/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
* This patch **fails PySpark unit tests**.
* This patch **does not merge cleanly**.
* This patch adds no public classes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792359649
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40428/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873990010
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140641/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r591058194
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
We're still discussing the solution in #30763. So I can't tell you the concrete split plan. But, I think, we'd be able to start with refactoring `MapStatus` either way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792236554
> Could you close the previous PR and update the PR title correctly?
Yes, updated this PR’s title. Will close previous PR after people start to review here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792357397
**[Test build #135846 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135846/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792362195
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40428/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873988690
**[Test build #140641 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140641/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
* This patch **fails Spark unit tests**.
* This patch **does not merge cleanly**.
* This patch adds no public classes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873990010
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140641/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589148712
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
@hiboyang Thanks for your explanation. I agree that #30763 is too big for review. But I think we can discuss there first to ensure we towards the same direction before we deep into details. And when we're on the same page, we can split the big PR into smaller pieces and start to co-work. Does it sound good to you?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589144809
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
> So next week I will do the conflict resolution and ping you when the PR is ready for review. Is this okay?
Sure, please. @attilapiros
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792381251
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135846/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873990010
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140641/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589060306
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
@Ngone51 I agree with you that finishing the design laid out in SPARK-25299 would be much better.
This is why I opened https://github.com/apache/spark/pull/30763 as a copy of Matthew Cheah's original PR for SPARK-31801 (because he is busy with other projects) and kept it up-to-date several times with the master.
But it haven't got enough reviews and I wouldn't want to block @hiboyang further, https://github.com/apache/spark/pull/30004#issuecomment-782915655.
I am sure with your help we can complete SPARK-31801 an be on the road of SPARK-25299.
So next week I will do the conflict resolution and ping you when the PR is ready for review. Is this okay?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589083966
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
Cool, thanks @attilapiros for keeping working on [SPARK-25299](https://issues.apache.org/jira/browse/SPARK-25299) while unblocking this PR. @Ngone51 [SPARK-33114](https://issues.apache.org/jira/browse/SPARK-33114) is a small change to support remote shuffle service/storage by adding a metadata object in MapStatus. It could be viewed as a subset of [SPARK-25299](https://issues.apache.org/jira/browse/SPARK-25299) 's work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791919257
Could you close the previous PR and update the PR title correctly?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589711145
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
[SPIP of SPARK-25299](https://docs.google.com/document/d/1d6egnL6WHOwWZe8MWv3m8n4PToNacdx7n_0iMSWwhCQ/edit#) says "API proposed here sits behind the existing SortShuffleManager implementation of the ShuffleManager interface". While we could continue the work in that area (behind the existing SortShuffleManager), could we also discuss from another point of view like supporting shuffle manager implementations other than SortShuffleManager?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589083033
##########
File path: core/src/test/scala/org/apache/spark/MapOutputTrackerSuite.scala
##########
@@ -146,13 +170,18 @@ class MapOutputTrackerSuite extends SparkFunSuite {
intercept[FetchFailedException] { mapWorkerTracker.getMapSizesByExecutorId(10, 0) }
val size1000 = MapStatus.decompressSize(MapStatus.compressSize(1000L))
- masterTracker.registerMapOutput(10, 0, MapStatus(
- BlockManagerId("a", "hostA", 1000), Array(1000L), 5))
+ val mapStatus = MapStatus(BlockManagerId("a", "hostA", 1000), Array(1000L), 5)
+ masterTracker.registerMapOutput(10, 0, mapStatus)
mapWorkerTracker.updateEpoch(masterTracker.getEpoch)
assert(mapWorkerTracker.getMapSizesByExecutorId(10, 0).toSeq ===
Seq((BlockManagerId("a", "hostA", 1000),
ArrayBuffer((ShuffleBlockId(10, 5, 0), size1000, 0)))))
- assert(0 == masterTracker.getNumCachedSerializedBroadcast)
Review comment:
it seems caused by merge issue, will add it back
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791912711
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135825/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-942812009
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873893785
**[Test build #140641 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140641/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791907483
**[Test build #135825 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135825/testReport)** for PR 31763 at commit [`1309378`](https://github.com/apache/spark/commit/1309378b570b1bd22a5a48c55cb078bb47f59846).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r591044762
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
Yeah, it is also good idea if we could split [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) and start with refactoring `MapStatus` first. Do you or the community get ideas about how to split [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589007510
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
hmm...what's the relationship between SPARK-33114 and SPARK-25299? According to the JIRA description, SPARK-33114 seems to enhance the support for custom shuffle manager while SPARK-25299 only customize the storage with the default `SortShuffleManager`.
So if we are only talking about SPARK-33114, adding `metadata` may be a good choice according to its own scenario. But if we bring in SPARK-25299 together (IIUC, what this PR is doing would also benefit SPARK-25299), I personally think we need a more general design here. For example, I'd prefer to redesign the `location` of `MapStatus` to make it be able to support different scenarios (e.g., Spark BlockManager, Spark external shuffle service, custom remote storage, etc. ) mentioned in SPARK-25299. And in this way, different scenarios would be able to reuse the existing features, e.g., decommission(which may update mapstatus location during runtime) and reuse the existing code paths, e.g., we don't need the extra `getAllMapOutputStatuses` and everything should be the same as what we already did during shuffle reading.
WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873990010
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140641/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873893785
**[Test build #140641 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140641/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791899103
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40407/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-821117623
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137478/
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-821117623
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137478/
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792360398
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40428/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-873893785
**[Test build #140641 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140641/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791892692
**[Test build #135825 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135825/testReport)** for PR 31763 at commit [`1309378`](https://github.com/apache/spark/commit/1309378b570b1bd22a5a48c55cb078bb47f59846).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589083067
##########
File path: core/src/main/scala/org/apache/spark/MapOutputTracker.scala
##########
@@ -774,6 +783,18 @@ private[spark] class MapOutputTrackerMaster(
}
}
+ def getAllMapOutputStatuses(shuffleId: Int): Array[MapStatus] = {
+ logDebug(s"Fetching all output statuses for shuffle $shuffleId")
+ shuffleStatuses.get(shuffleId) match {
+ case Some(shuffleStatus) =>
+ shuffleStatus.withMapStatuses { statuses =>
+ MapOutputTracker.checkMapStatuses(statuses, shuffleId)
+ statuses.clone
Review comment:
Cool, I will change it `getAllMapOutputStatusMetadata` and update this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r590959025
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
I don't think we should develop like this way...As you mentioned above, SPARK-33114 can be considered as a subtask of SPARK-25299. So how can we consider this PR as a first iteration when SPARK-25299 is still under discussion and development, especially when people haven't reached an agreement on the solution and has a possible alternative solution at the same time? Also, I think the custom shuffle manager isn't officially supported by Spark because the `ShuffleManager` interface is private. So it doesn't make sense for Spark to add an internal API for un-official use cases if there's no strong reason.
SPARK-31801 is surely big. But as I mentioned early, we can split it. When the solution is finalized, we can start with refactoring `MapStatus` first. I think it would be a much smaller task and be enough for your case. And then, we'll start the remaining work(e.g. use the new `MapStatus` where it was referenced) but you don't care.
I understand you have paid a lot of effort into this work, and sorry we can not get it in fast. And, unfortunately, I don't have the permission to merge. You could persuade committers to merge the PR if you insist on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r589899892
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
Then, it'd be a totally different topic, right? IIUC, SPARK-25299 could also benefit custom shuffle manager if SPARK-25299(custom storage) is pluggable with a custom shuffle manager. Ideally, a custom shuffle manager should be able to plugin in different storages. We might not think deeply about how to support custom shuffle managers when working on SPARK-25299 but need to keep in mind the "pluggable". After SPARK-25299 completed, then, we can start to enhance the support for custom shuffle managers.
That being said, I think you are still free to raise a separate discussion on supporting the custom shuffle manager. For example, what are the shortcomings of the current framework, and what should be improved...I just wonder the community may not have enough bandwidth to work on these two significant projects concurently.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r588996218
##########
File path: core/src/main/scala/org/apache/spark/MapOutputTracker.scala
##########
@@ -774,6 +783,18 @@ private[spark] class MapOutputTrackerMaster(
}
}
+ def getAllMapOutputStatuses(shuffleId: Int): Array[MapStatus] = {
+ logDebug(s"Fetching all output statuses for shuffle $shuffleId")
+ shuffleStatuses.get(shuffleId) match {
+ case Some(shuffleStatus) =>
+ shuffleStatus.withMapStatuses { statuses =>
+ MapOutputTracker.checkMapStatuses(statuses, shuffleId)
+ statuses.clone
Review comment:
So as we discussed in https://github.com/apache/spark/pull/30004#discussion_r588867352
To change it to `getAllMapOutputStatusMetadata` and only return the metadata could be a solution extended with the restriction to allow only immutable metadata.
>So to be on the safe side please document we require the metadata to be immutable and introduce an updateMetadata(meta: Option[Serializable]) method in MapStatus. Then we will be safe and all the use cases are covered.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hiboyang commented on a change in pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
hiboyang commented on a change in pull request #31763:
URL: https://github.com/apache/spark/pull/31763#discussion_r590917443
##########
File path: core/src/main/scala/org/apache/spark/scheduler/MapStatus.scala
##########
@@ -52,6 +52,13 @@ private[spark] sealed trait MapStatus {
* partitionId of the task or taskContext.taskAttemptId is used.
*/
def mapId: Long
+
+ /**
+ * Extra metadata for map status. This could be used by different ShuffleManager implementation
+ * to store information they need. For example, a Remote Shuffle Service ShuffleManager could
+ * store shuffle server information and let reducer task know where to fetch shuffle data.
+ */
+ def metadata: Option[Serializable]
Review comment:
I feel [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) is very big, and may take very long time to finish. Could we merge this PR first?
If [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) find a better way to support it and break `getAllMapOutputStatusMetadata`, it is actually a good thing :) We could have multiple iterations. This PR is the first iteration with very small change. [SPARK-31801](https://issues.apache.org/jira/browse/SPARK-31801) is the iteration after that. The latter does not need to block the former one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-792378972
**[Test build #135846 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135846/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31763: Map status metadata2
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-791892692
**[Test build #135825 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/135825/testReport)** for PR 31763 at commit [`1309378`](https://github.com/apache/spark/commit/1309378b570b1bd22a5a48c55cb078bb47f59846).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31763: [SPARK-33114][CORE] Add metadata in MapStatus to support custom shuffle manager
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31763:
URL: https://github.com/apache/spark/pull/31763#issuecomment-821022941
**[Test build #137478 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137478/testReport)** for PR 31763 at commit [`798addf`](https://github.com/apache/spark/commit/798addff0c90228995776d746421284df2cfa6c7).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org