You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by colorant <gi...@git.apache.org> on 2014/06/27 04:19:32 UTC
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
GitHub user colorant opened a pull request:
https://github.com/apache/spark/pull/1241
[SPARK-2288] Hide ShuffleBlockManager behind ShuffleManager
By Hiding the shuffleblockmanager behind Shufflemanager, we decouple the shuffle data's block mapping management work from Diskblockmananger. This give a more clear interface and more easy for other shuffle manager to implement their own block management logic. the jira ticket have more details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/colorant/spark shuffle
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1241.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1241
----
commit 209921361ff80812e95cd640ae66013614fb46b9
Author: Raymond Liu <ra...@intel.com>
Date: 2014-06-26T07:15:56Z
Move ShuffleBlockmanager behind shuffleManager
commit 1d606985b9307503d0e61755bdd2ee41a61bc94f
Author: Raymond Liu <ra...@intel.com>
Date: 2014-06-26T07:42:44Z
Fix test cases
commit d024134bd501d88ccb450f25cf6fb71478005cae
Author: Raymond Liu <ra...@intel.com>
Date: 2014-06-26T08:26:09Z
Fix getBlockLocation
commit 8b885529f639bd0c01dc58f2ca6376cf0ea80a0a
Author: Raymond Liu <ra...@intel.com>
Date: 2014-06-27T00:51:18Z
Fix code style
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53368876
@mateiz @rxin , rebased upon netty shuffle changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760213
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
--- End diff --
also i don't think conf is used at all? maybe remove it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-50737111
QA results for PR 1241:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17572/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48284012
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48571955
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16493/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759807
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with PathResolver {
+
+ private lazy val blockManager = SparkEnv.get.blockManager
+
+ // Mapping to a single shuffleBlockId with reduce ID 0 that we'll used to write all results to.
+ private def consolidatedId(blockId: ShuffleBlockId): ShuffleBlockId = {
+ blockId.copy(reduceId = 0)
+ }
+
+ def getDataFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId))
+ }
+
+ private def getIndexFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId).name + ".index")
+ }
+
+ // Remove data file and index file that contain the output data from one map.
+ // Only need to be called once for all shuffleBlockId belong to one map output.
+ def removeDataByMap(blockId: ShuffleBlockId): Unit = {
--- End diff --
this is a fairly awkard way of specifying a map .. why don't you just take the mapId in?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47307223
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16190/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47303494
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696089
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleManager.scala ---
@@ -24,7 +24,10 @@ import org.apache.spark.shuffle._
* A ShuffleManager using hashing, that creates one output file per reduce partition on each
* mapper (possibly reusing these across waves of tasks).
*/
-class HashShuffleManager(conf: SparkConf) extends ShuffleManager {
+class HashShuffleManager(conf: SparkConf) extends ShuffleManager with Logging{
--- End diff --
space after Logging
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696083
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleManager.scala ---
@@ -49,8 +49,12 @@ private[spark] trait ShuffleManager {
endPartition: Int,
context: TaskContext): ShuffleReader[K, C]
- /** Remove a shuffle's metadata from the ShuffleManager. */
- def unregisterShuffle(shuffleId: Int)
+ /** Remove a shuffle's metadata from the ShuffleManager.
--- End diff --
Yes, the BlockManagerSlaveActor is using it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48558914
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759777
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with PathResolver {
+
+ private lazy val blockManager = SparkEnv.get.blockManager
+
+ // Mapping to a single shuffleBlockId with reduce ID 0 that we'll used to write all results to.
+ private def consolidatedId(blockId: ShuffleBlockId): ShuffleBlockId = {
+ blockId.copy(reduceId = 0)
+ }
+
+ def getDataFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId))
+ }
+
+ private def getIndexFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId).name + ".index")
+ }
+
+ // Remove data file and index file that contain the output data from one map.
+ // Only need to be called once for all shuffleBlockId belong to one map output.
+ def removeDataByMap(blockId: ShuffleBlockId): Unit = {
+ var file = getDataFile(blockId)
+ if (file.exists) {
+ file.delete
--- End diff --
add parenthesis since delete has side effect
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47311892
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47303485
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48284014
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16396/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696062
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleBlockManager.scala ---
@@ -156,17 +162,25 @@ class ShuffleBlockManager(blockManager: BlockManager) extends Logging {
/**
* Returns the physical file segment in which the given BlockId is located.
- * This function should only be called if shuffle file consolidation is enabled, as it is
- * an error condition if we don't find the expected block.
*/
def getBlockLocation(id: ShuffleBlockId): FileSegment = {
- // Search all file groups associated with this shuffle.
- val shuffleState = shuffleStates(id.shuffleId)
- for (fileGroup <- shuffleState.allFileGroups) {
- val segment = fileGroup.getFileSegmentFor(id.mapId, id.reduceId)
- if (segment.isDefined) { return segment.get }
+ if (consolidateShuffleFiles) {
+ // Search all file groups associated with this shuffle.
+ val shuffleState = shuffleStates(id.shuffleId)
+ for (fileGroup <- shuffleState.allFileGroups) {
--- End diff --
While you are at it, do you mind rewriting this into a while loop? It gets expensive to do "return" within a for closure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696114
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleWriter.scala ---
@@ -25,6 +25,7 @@ import org.apache.spark.executor.ShuffleWriteMetrics
import org.apache.spark.scheduler.MapStatus
class HashShuffleWriter[K, V](
+ hashShuffleBlockManager: HashShuffleBlockManager,
--- End diff --
we can just name this shuffleBlockManager since hash is implied already. that shortens your code slightly ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-50710410
@rxin @mateiz , I have rebase this on top of sort-base-shuffle. Would you mind to take a review again?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759644
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala ---
@@ -121,20 +129,16 @@ private[spark] class DiskStore(blockManager: BlockManager, diskManager: DiskBloc
}
override def remove(blockId: BlockId): Boolean = {
- val fileSegment = diskManager.getBlockLocation(blockId)
- val file = fileSegment.file
- if (file.exists() && file.length() == fileSegment.length) {
+ val file = diskManager.getFile(blockId.name)
--- End diff --
this changes the behavior to always delete the file even if consolidation is on. Is this intended / correct?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759276
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -573,7 +634,7 @@ private[spark] class BlockManager(
serializer: Serializer,
readMetrics: ShuffleReadMetrics): BlockFetcherIterator = {
val iter =
- if (conf.getBoolean("spark.shuffle.use.netty", false)) {
+ if (useNetty) {
--- End diff --
indent is off here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53522043
Thanks for keeping this up to date @colorant -- going to take a look soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53368902
Jenkins, retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759963
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
--- End diff --
this import is redundant
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16825748
--- Diff: core/src/main/scala/org/apache/spark/shuffle/FileShuffleBlockManager.scala ---
@@ -181,17 +171,30 @@ class ShuffleBlockManager(blockManager: BlockManager,
/**
* Returns the physical file segment in which the given BlockId is located.
- * This function should only be called if shuffle file consolidation is enabled, as it is
- * an error condition if we don't find the expected block.
*/
def getBlockLocation(id: ShuffleBlockId): FileSegment = {
- // Search all file groups associated with this shuffle.
- val shuffleState = shuffleStates(id.shuffleId)
- for (fileGroup <- shuffleState.allFileGroups) {
- val segment = fileGroup.getFileSegmentFor(id.mapId, id.reduceId)
- if (segment.isDefined) { return segment.get }
+ if (consolidateShuffleFiles) {
+ // Search all file groups associated with this shuffle.
+ val shuffleState = shuffleStates(id.shuffleId)
+ val iter = shuffleState.allFileGroups.iterator
+ while (iter.hasNext) {
+ val segment = iter.next.getFileSegmentFor(id.mapId, id.reduceId)
+ if (segment.isDefined) { return segment.get }
+ }
+ throw new IllegalStateException("Failed to find shuffle block: " + id)
+ } else {
+ val file = blockManager.diskBlockManager.getFile(id)
--- End diff --
you mean diskBlockManager.getfile? yes, it know nothing about consolidation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48569956
Actually it's still necessary once the PR gets merged. It basically compares all the public interfaces at bytecode level against 1.0, and see if any public interfaces changed. This is done in place to ensure API compatibility throughout the 1.x release cycle.
Unfortunately, all the methods that are labeled "private[package_name]" loses the private visibility at bytecode level, so the compatibility checker is more conservative and we would need to manually mark these interface changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48576865
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16493/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696298
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -72,23 +74,48 @@ private[spark] class BlockManager(
// Actual storage of where blocks are kept
private var tachyonInitialized = false
private[storage] val memoryStore = new MemoryStore(this, maxMemory)
- private[storage] val diskStore = new DiskStore(this, diskBlockManager)
+ private[spark] val diskStore = new DiskStore(this, diskBlockManager)
private[storage] lazy val tachyonStore: TachyonStore = {
val storeDir = conf.get("spark.tachyonStore.baseDir", "/tmp_spark_tachyon")
val appFolderName = conf.get("spark.tachyonStore.folderName")
val tachyonStorePath = s"$storeDir/$appFolderName/${this.executorId}"
val tachyonMaster = conf.get("spark.tachyonStore.url", "tachyon://localhost:19998")
val tachyonBlockManager =
- new TachyonBlockManager(shuffleBlockManager, tachyonStorePath, tachyonMaster)
+ new TachyonBlockManager(this, tachyonStorePath, tachyonMaster)
tachyonInitialized = true
new TachyonStore(this, tachyonBlockManager)
}
+ val shuffleManager = {
+ val name = conf.get("spark.shuffle.manager",
+ "org.apache.spark.shuffle.hash.HashShuffleManager")
+ val clazz = Class.forName(name, true, Utils.getContextOrSparkClassLoader)
+ try {
+ clazz.getConstructor(classOf[SparkConf]).newInstance(conf).asInstanceOf[ShuffleManager]
+ } catch {
+ case _: NoSuchMethodException =>
+ clazz.getConstructor().newInstance().asInstanceOf[ShuffleManager]
+ }
+ }
+
+ private var nettyShuffleSender : ShuffleSender = null
// If we use Netty for shuffle, start a new Netty-based shuffle sender service.
private val nettyPort: Int = {
val useNetty = conf.getBoolean("spark.shuffle.use.netty", false)
val nettyPortConfig = conf.getInt("spark.shuffle.sender.port", 0)
- if (useNetty) diskBlockManager.startShuffleBlockSender(nettyPortConfig) else 0
+ // At present, only HashShuffleBlockManager support the getBlockLocation method.
+ if (useNetty && shuffleManager.isInstanceOf[HashShuffleManager]) {
--- End diff --
right now we have two comm implementations in Spark: netty and a custom nio implementation. After 1.1, we should look into the two and consolidate them. We should pick one and make sure that is robust, instead of having two that complicate things.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-50732503
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17572/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47495493
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696261
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -72,23 +74,48 @@ private[spark] class BlockManager(
// Actual storage of where blocks are kept
private var tachyonInitialized = false
private[storage] val memoryStore = new MemoryStore(this, maxMemory)
- private[storage] val diskStore = new DiskStore(this, diskBlockManager)
+ private[spark] val diskStore = new DiskStore(this, diskBlockManager)
private[storage] lazy val tachyonStore: TachyonStore = {
val storeDir = conf.get("spark.tachyonStore.baseDir", "/tmp_spark_tachyon")
val appFolderName = conf.get("spark.tachyonStore.folderName")
val tachyonStorePath = s"$storeDir/$appFolderName/${this.executorId}"
val tachyonMaster = conf.get("spark.tachyonStore.url", "tachyon://localhost:19998")
val tachyonBlockManager =
- new TachyonBlockManager(shuffleBlockManager, tachyonStorePath, tachyonMaster)
+ new TachyonBlockManager(this, tachyonStorePath, tachyonMaster)
tachyonInitialized = true
new TachyonStore(this, tachyonBlockManager)
}
+ val shuffleManager = {
--- End diff --
Let me add to this - blockmanager in spark doesn't really have a clean abstraction. This is something we should improve over time. I'd be wary to break the abstraction further ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52010877
QA results for PR 1241:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18417/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47491610
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696621
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -72,23 +74,48 @@ private[spark] class BlockManager(
// Actual storage of where blocks are kept
private var tachyonInitialized = false
private[storage] val memoryStore = new MemoryStore(this, maxMemory)
- private[storage] val diskStore = new DiskStore(this, diskBlockManager)
+ private[spark] val diskStore = new DiskStore(this, diskBlockManager)
private[storage] lazy val tachyonStore: TachyonStore = {
val storeDir = conf.get("spark.tachyonStore.baseDir", "/tmp_spark_tachyon")
val appFolderName = conf.get("spark.tachyonStore.folderName")
val tachyonStorePath = s"$storeDir/$appFolderName/${this.executorId}"
val tachyonMaster = conf.get("spark.tachyonStore.url", "tachyon://localhost:19998")
val tachyonBlockManager =
- new TachyonBlockManager(shuffleBlockManager, tachyonStorePath, tachyonMaster)
+ new TachyonBlockManager(this, tachyonStorePath, tachyonMaster)
tachyonInitialized = true
new TachyonStore(this, tachyonBlockManager)
}
+ val shuffleManager = {
--- End diff --
Will try my best to see whether it is possible and not too tricky to keep it out of Block Manager.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47307220
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47495499
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16256/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14279781
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleBlockManager.scala ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import org.apache.spark.storage.{FileSegment, ShuffleBlockId}
+import java.nio.ByteBuffer
+
+private[spark]
+trait ShuffleBlockManager {
--- End diff --
yeah, I see your concern, I thought about this too. actually in this PR, it mostly is used internally by HashShuffleManager's writter. While, we can take this as a way to give a chance to expose the internal storage objects for short cut usage. Such as current netty based shuffle sender. Without this interface, it's hard to implement without introduce maybe many more extra interface. to keep it simple. I offer the chance to expose the ShuffleBlockManager.
And then, this "location" conception might not be meaningful, but and BlockObjectId might be a good fit for all the possible shuffleManager, afterall, you are handling some objects whether it is a File , or a Stream, or whatever way you save your data to, So, a ShuffleBlockManager it self might still be needed to access this object in certain shortcut cases for simplifier API, and you can name the method getDataObjectHandle or whatever fits.. I do also have a PR for this idea, say generalize the object and pass around an ObjectID for different storage type at #1209
So does this make any sense to you ;) Still, I agree if we could find better way to solve the netty block fetcher problem, This could be hide.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759577
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala ---
@@ -89,25 +89,33 @@ private[spark] class DiskStore(blockManager: BlockManager, diskManager: DiskBloc
}
}
- override def getBytes(blockId: BlockId): Option[ByteBuffer] = {
- val segment = diskManager.getBlockLocation(blockId)
- val channel = new RandomAccessFile(segment.file, "r").getChannel
+ private def getBytes(file: File, offset: Long, length: Long): Option[ByteBuffer] = {
+ val channel = new RandomAccessFile(file, "r").getChannel
try {
// For small files, directly read rather than memory map
- if (segment.length < minMemoryMapBytes) {
- val buf = ByteBuffer.allocate(segment.length.toInt)
- channel.read(buf, segment.offset)
+ if (file.length < minMemoryMapBytes) {
+ val buf = ByteBuffer.allocate(length.toInt)
+ channel.read(buf, offset)
buf.flip()
Some(buf)
} else {
- Some(channel.map(MapMode.READ_ONLY, segment.offset, segment.length))
+ Some(channel.map(MapMode.READ_ONLY, offset, length))
}
} finally {
channel.close()
}
}
+ override def getBytes(blockId: BlockId): Option[ByteBuffer] = {
+ val file = diskManager.getFile(blockId.name)
+ getBytes(file, 0, file.length)
+ }
+
+ def getBytes(segment: FileSegment): Option[ByteBuffer] = {
+ getBytes(segment.file, segment.offset, segment.length)
+ }
+
override def getValues(blockId: BlockId): Option[Iterator[Any]] = {
--- End diff --
can u add a todo here that getValues should bypass getBytes to use stream based APIs? Otherwise this uses a lot of memory during external sort merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48280591
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760032
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
--- End diff --
can u add javadoc explaining what this class does ... and the structure of the blocks (e.g. reduce id 0 is where we put all the blocks)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-50725957
@colorant thanks for doing this, but we should probably wait to merge it until after 1.1, since it doesn't add critical fixes for 1.1. I'll take a look at it after I'm done reviewing some 1.1 stuff.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1241
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759047
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleBlockManager.scala ---
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import org.apache.spark.storage.{FileSegment, ShuffleBlockId}
--- End diff --
sort the import
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47311880
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48280584
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48135413
ping @rxin ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48561260
QA results for PR 1241:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16486/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759699
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleBlockManager.scala ---
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import org.apache.spark.storage.{FileSegment, ShuffleBlockId}
+import java.nio.ByteBuffer
+
+private[spark]
+trait ShuffleBlockManager {
+ type ShuffleId = Int
+
+ def getBytes(blockId: ShuffleBlockId): Option[ByteBuffer]
--- End diff --
can you add some javadoc explaining what this method is for?
e.g. is it for getting local blocks or remote blocks? In what condition does it return Some(buf) vs None?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48567726
I think you can edit project/MimaExcludes.scala file to fix that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47314420
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16199/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48576862
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696189
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -72,23 +74,48 @@ private[spark] class BlockManager(
// Actual storage of where blocks are kept
private var tachyonInitialized = false
private[storage] val memoryStore = new MemoryStore(this, maxMemory)
- private[storage] val diskStore = new DiskStore(this, diskBlockManager)
+ private[spark] val diskStore = new DiskStore(this, diskBlockManager)
private[storage] lazy val tachyonStore: TachyonStore = {
val storeDir = conf.get("spark.tachyonStore.baseDir", "/tmp_spark_tachyon")
val appFolderName = conf.get("spark.tachyonStore.folderName")
val tachyonStorePath = s"$storeDir/$appFolderName/${this.executorId}"
val tachyonMaster = conf.get("spark.tachyonStore.url", "tachyon://localhost:19998")
val tachyonBlockManager =
- new TachyonBlockManager(shuffleBlockManager, tachyonStorePath, tachyonMaster)
+ new TachyonBlockManager(this, tachyonStorePath, tachyonMaster)
tachyonInitialized = true
new TachyonStore(this, tachyonBlockManager)
}
+ val shuffleManager = {
--- End diff --
It would be best if we can avoid putting the shuffle manager in BlockManager. This violates abstraction.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759331
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala ---
@@ -89,25 +89,33 @@ private[spark] class DiskStore(blockManager: BlockManager, diskManager: DiskBloc
}
}
- override def getBytes(blockId: BlockId): Option[ByteBuffer] = {
- val segment = diskManager.getBlockLocation(blockId)
- val channel = new RandomAccessFile(segment.file, "r").getChannel
+ private def getBytes(file: File, offset: Long, length: Long): Option[ByteBuffer] = {
+ val channel = new RandomAccessFile(file, "r").getChannel
try {
// For small files, directly read rather than memory map
- if (segment.length < minMemoryMapBytes) {
- val buf = ByteBuffer.allocate(segment.length.toInt)
- channel.read(buf, segment.offset)
+ if (file.length < minMemoryMapBytes) {
--- End diff --
I think u want length instead of file.length here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47302193
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16183/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52014668
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696085
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleManager.scala ---
@@ -49,12 +52,21 @@ class HashShuffleManager(conf: SparkConf) extends ShuffleManager {
/** Get a writer for a given partition. Called on executors by map tasks. */
override def getWriter[K, V](handle: ShuffleHandle, mapId: Int, context: TaskContext)
: ShuffleWriter[K, V] = {
- new HashShuffleWriter(handle.asInstanceOf[BaseShuffleHandle[K, V, _]], mapId, context)
+ new HashShuffleWriter(
+ hashShuffleBlockManager,handle.asInstanceOf[BaseShuffleHandle[K, V, _]], mapId, context)
--- End diff --
space before handle.asInstanceOf[BaseShuffleHandle
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53841329
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52016533
QA results for PR 1241:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18425/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759266
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -107,6 +153,11 @@ private[spark] class BlockManager(
}
}
+ if (useNetty && (shuffleManager.isInstanceOf[HashShuffleManager]
+ || shuffleManager.isInstanceOf[SortShuffleManager])) {
+ val resolver = shuffleManager.shuffleBlockManager.asInstanceOf[PathResolver]
--- End diff --
does this actually do anything?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759141
--- Diff: core/src/main/scala/org/apache/spark/shuffle/sort/SortShuffleManager.scala ---
@@ -17,14 +17,18 @@
package org.apache.spark.shuffle.sort
-import java.io.{DataInputStream, FileInputStream}
+import java.util.concurrent.ConcurrentHashMap
+import org.apache.spark.{SparkConf, TaskContext, ShuffleDependency}
import org.apache.spark.shuffle._
-import org.apache.spark.{TaskContext, ShuffleDependency}
import org.apache.spark.shuffle.hash.HashShuffleReader
-import org.apache.spark.storage.{DiskBlockManager, FileSegment, ShuffleBlockId}
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark] class SortShuffleManager(conf: SparkConf) extends ShuffleManager {
+
+ val indexShuffleBlockManager = new IndexShuffleBlockManager(conf)
--- End diff --
tighten the accessibility - make everything private
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759235
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -34,8 +34,11 @@ import org.apache.spark.io.CompressionCodec
import org.apache.spark.network._
import org.apache.spark.network.netty.client.BlockFetchingClientFactory
import org.apache.spark.network.netty.server.BlockServer
+import org.apache.spark.network.netty.PathResolver
--- End diff --
sort the imports
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14278945
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleBlockManager.scala ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import org.apache.spark.storage.{FileSegment, ShuffleBlockId}
+import java.nio.ByteBuffer
+
+private[spark]
+trait ShuffleBlockManager {
--- End diff --
I don't know if you want this to be a top level interface since it ties the notion of block locations directly into shuffle, whereas the current design only requires write/read.
For example, if we use an external storage system to handle shuffle, location might not be a meaningful concept...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47495494
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48683259
@rxin Any thing else I can do? ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48563750
Test fail is due to API changes in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53844784
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19454/consoleFull) for PR 1241 at commit [`0e01ae3`](https://github.com/apache/spark/commit/0e01ae321735aea5e4f568f247968e68e7f3ec33).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759871
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with PathResolver {
+
+ private lazy val blockManager = SparkEnv.get.blockManager
+
+ // Mapping to a single shuffleBlockId with reduce ID 0 that we'll used to write all results to.
+ private def consolidatedId(blockId: ShuffleBlockId): ShuffleBlockId = {
+ blockId.copy(reduceId = 0)
+ }
+
+ def getDataFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId))
+ }
+
+ private def getIndexFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId).name + ".index")
+ }
+
+ // Remove data file and index file that contain the output data from one map.
+ // Only need to be called once for all shuffleBlockId belong to one map output.
+ def removeDataByMap(blockId: ShuffleBlockId): Unit = {
+ var file = getDataFile(blockId)
+ if (file.exists) {
+ file.delete
+ }
+
+ file = getIndexFile(blockId)
+ if (file.exists) {
+ file.delete
+ }
+ }
+
+ // Write an index file with the offsets of each block, plus a final offset at the end for the
+ // end of the output file. This will be used by getBlockLocation to figure out where each block
+ // begins and ends.
+ def writeIndexFile(blockId: ShuffleBlockId, offsets: Array[Long]) = {
+ val indexFile = getIndexFile(blockId)
+ val out = new DataOutputStream(new BufferedOutputStream(new FileOutputStream(indexFile)))
+ try {
+ for(offset <- offsets) {
--- End diff --
space after for
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47302192
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47493571
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760326
--- Diff: core/src/main/scala/org/apache/spark/shuffle/FileShuffleBlockManager.scala ---
@@ -181,17 +171,30 @@ class ShuffleBlockManager(blockManager: BlockManager,
/**
* Returns the physical file segment in which the given BlockId is located.
- * This function should only be called if shuffle file consolidation is enabled, as it is
- * an error condition if we don't find the expected block.
*/
def getBlockLocation(id: ShuffleBlockId): FileSegment = {
- // Search all file groups associated with this shuffle.
- val shuffleState = shuffleStates(id.shuffleId)
- for (fileGroup <- shuffleState.allFileGroups) {
- val segment = fileGroup.getFileSegmentFor(id.mapId, id.reduceId)
- if (segment.isDefined) { return segment.get }
+ if (consolidateShuffleFiles) {
+ // Search all file groups associated with this shuffle.
+ val shuffleState = shuffleStates(id.shuffleId)
+ val iter = shuffleState.allFileGroups.iterator
+ while (iter.hasNext) {
+ val segment = iter.next.getFileSegmentFor(id.mapId, id.reduceId)
+ if (segment.isDefined) { return segment.get }
+ }
+ throw new IllegalStateException("Failed to find shuffle block: " + id)
+ } else {
+ val file = blockManager.diskBlockManager.getFile(id)
--- End diff --
this no longer supports file consolidation, does it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14695993
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleBlockManager.scala ---
@@ -59,8 +64,10 @@ private[spark] trait ShuffleWriterGroup {
* files within a ShuffleFileGroups associated with the block's reducer.
*/
private[spark]
-class ShuffleBlockManager(blockManager: BlockManager) extends Logging {
- def conf = blockManager.conf
+class HashShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with Logging {
+
+ lazy val blockManager = SparkEnv.get.blockManager
--- End diff --
private?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47301712
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760248
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleManager.scala ---
@@ -25,6 +25,9 @@ import org.apache.spark.shuffle._
* mapper (possibly reusing these across waves of tasks).
*/
private[spark] class HashShuffleManager(conf: SparkConf) extends ShuffleManager {
+
+ val fileShuffleBlockManager = new FileShuffleBlockManager(conf)
--- End diff --
private
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14282384
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleBlockManager.scala ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import org.apache.spark.storage.{FileSegment, ShuffleBlockId}
+import java.nio.ByteBuffer
+
+private[spark]
+trait ShuffleBlockManager {
--- End diff --
@rxin, How about we also hide current BlockFetcherIterator kind of thing behind shuffleManager. since a specific shuffleManager not necessary using current fetcher approaching to get shuffle data. Each shuffleManager should instance his own shuffle logic, while some could reuse the same logic, say FileBased one could reuse current implementation. By this way, we can solve the above problem and have better chance to not expose shuffleBlockManager, say a read/write interface for shuffle reader/writter is enough.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760826
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -107,6 +153,11 @@ private[spark] class BlockManager(
}
}
+ if (useNetty && (shuffleManager.isInstanceOf[HashShuffleManager]
+ || shuffleManager.isInstanceOf[SortShuffleManager])) {
+ val resolver = shuffleManager.shuffleBlockManager.asInstanceOf[PathResolver]
--- End diff --
yeap, suppose other non file based shuffleBlockManager not implement PathResolver
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16763705
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala ---
@@ -121,20 +129,16 @@ private[spark] class DiskStore(blockManager: BlockManager, diskManager: DiskBloc
}
override def remove(blockId: BlockId): Boolean = {
- val fileSegment = diskManager.getBlockLocation(blockId)
- val file = fileSegment.file
- if (file.exists() && file.length() == fileSegment.length) {
+ val file = diskManager.getFile(blockId.name)
--- End diff --
Yep, for consolidation. But think again for sortShuffleManager, since all data currently is stored in reduiceId 0 file, then it will lead to some problem. I am wondering to change the physical filename mapping there too to solve this problem. since we won't want disk manager to know anything about shuffle block name mapping.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759291
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -573,7 +634,7 @@ private[spark] class BlockManager(
serializer: Serializer,
readMetrics: ShuffleReadMetrics): BlockFetcherIterator = {
val iter =
- if (conf.getBoolean("spark.shuffle.use.netty", false)) {
+ if (useNetty) {
--- End diff --
but fyi .. as part of https://issues.apache.org/jira/browse/SPARK-3019 I'm cleaning all these stuff up and there won't be any Netty specific code anymore, so it's probably ok to not clean this up here (but don't worry about it since you have done it already)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47493840
@rxin Moved getBlockLocation method from shuffleBlockManager to HashShuffleBlockMananger to make the interface more general. Does current interface looks reasonable for you?
Also still a few shuffle related code could be moved further from block manager to some specific shuffle manager related classes' implementation ( e.g. blockManager.getMultiple). But since they are not tightly related to this shuffleBlockManager generalization works and I am not quite sure whether the other shufflemanager implementation will reuse them or not, so just leave it as it is, and could be done in future PR I guess.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53841355
Jenkins, ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48571873
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16762641
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala ---
@@ -121,20 +129,16 @@ private[spark] class DiskStore(blockManager: BlockManager, diskManager: DiskBloc
}
override def remove(blockId: BlockId): Boolean = {
- val fileSegment = diskManager.getBlockLocation(blockId)
- val file = fileSegment.file
- if (file.exists() && file.length() == fileSegment.length) {
+ val file = diskManager.getFile(blockId.name)
--- End diff --
are you saying diskManager.getFile would return a file that doens't exist when consolidation is on? if that's the case, can u add inline comment explaining this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696031
--- Diff: core/src/main/scala/org/apache/spark/shuffle/hash/HashShuffleBlockManager.scala ---
@@ -59,8 +64,10 @@ private[spark] trait ShuffleWriterGroup {
* files within a ShuffleFileGroups associated with the block's reducer.
*/
private[spark]
-class ShuffleBlockManager(blockManager: BlockManager) extends Logging {
- def conf = blockManager.conf
+class HashShuffleBlockManager(conf: SparkConf)
--- End diff --
One thing about this naming is that the block manager technically isn't specific to hashing. For example, we can still do sort-based shuffle using this block manager (although that's unlikely to happen).
@aarondav any thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52014901
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18425/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48569640
@rxin you mean by add ProblemFilters.exclude[MissingMethodProblem]("org.apache.spark.storage.DiskStore.getValues")?
Then, this will not be needed for later usage as long as this PR got merged in? Seems to me a little bit strange to add this kind of one time staff. Is that the standard process?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48571858
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760004
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with PathResolver {
+
+ private lazy val blockManager = SparkEnv.get.blockManager
+
+ // Mapping to a single shuffleBlockId with reduce ID 0 that we'll used to write all results to.
+ private def consolidatedId(blockId: ShuffleBlockId): ShuffleBlockId = {
+ blockId.copy(reduceId = 0)
+ }
+
+ def getDataFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId))
+ }
+
+ private def getIndexFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId).name + ".index")
+ }
+
+ // Remove data file and index file that contain the output data from one map.
--- End diff --
use /** */ instead of //
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52011647
hmm, a local RAT check did pass.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759788
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with PathResolver {
+
+ private lazy val blockManager = SparkEnv.get.blockManager
+
+ // Mapping to a single shuffleBlockId with reduce ID 0 that we'll used to write all results to.
+ private def consolidatedId(blockId: ShuffleBlockId): ShuffleBlockId = {
+ blockId.copy(reduceId = 0)
+ }
+
+ def getDataFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId))
+ }
+
+ private def getIndexFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId).name + ".index")
+ }
+
+ // Remove data file and index file that contain the output data from one map.
+ // Only need to be called once for all shuffleBlockId belong to one map output.
+ def removeDataByMap(blockId: ShuffleBlockId): Unit = {
+ var file = getDataFile(blockId)
+ if (file.exists) {
+ file.delete
+ }
+
+ file = getIndexFile(blockId)
+ if (file.exists) {
--- End diff --
add parenthesis since exists / delete have side effect (in the case of exists it looks up the inode)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16762489
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala ---
@@ -121,20 +129,16 @@ private[spark] class DiskStore(blockManager: BlockManager, diskManager: DiskBloc
}
override def remove(blockId: BlockId): Boolean = {
- val fileSegment = diskManager.getBlockLocation(blockId)
- val file = fileSegment.file
- if (file.exists() && file.length() == fileSegment.length) {
+ val file = diskManager.getFile(blockId.name)
--- End diff --
no, it won't change the behavior, when consolidation is on, the physical file name is different from blockId.name, so it won't delete anything.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759256
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -85,18 +87,62 @@ private[spark] class BlockManager(
val tachyonStorePath = s"$storeDir/$appFolderName/${this.executorId}"
val tachyonMaster = conf.get("spark.tachyonStore.url", "tachyon://localhost:19998")
val tachyonBlockManager =
- new TachyonBlockManager(shuffleBlockManager, tachyonStorePath, tachyonMaster)
+ new TachyonBlockManager(this, tachyonStorePath, tachyonMaster)
tachyonInitialized = true
new TachyonStore(this, tachyonBlockManager)
}
- private val useNetty = conf.getBoolean("spark.shuffle.use.netty", false)
+ val shuffleManager = {
+ // Let the user specify short names for shuffle managers
+ val shortShuffleMgrNames = Map(
+ "hash" -> "org.apache.spark.shuffle.hash.HashShuffleManager",
+ "sort" -> "org.apache.spark.shuffle.sort.SortShuffleManager")
+ val shuffleMgrName = conf.get("spark.shuffle.manager", "hash")
+ val shuffleMgrClass = shortShuffleMgrNames.getOrElse(shuffleMgrName.toLowerCase, shuffleMgrName)
+
+ val clazz = Class.forName(shuffleMgrClass, true, Utils.getContextOrSparkClassLoader)
+ try {
+ clazz.getConstructor(classOf[SparkConf]).newInstance(conf).asInstanceOf[ShuffleManager]
+ } catch {
+ case _: NoSuchMethodException =>
+ clazz.getConstructor().newInstance().asInstanceOf[ShuffleManager]
+ }
+ }
+
+ var shuffleBlockPathResolver: PathResolver = null
+ private val useNetty = {
+ val userConfigNetty = conf.getBoolean("spark.shuffle.use.netty", false)
+
+ if (userConfigNetty && (shuffleManager.isInstanceOf[HashShuffleManager]
+ || shuffleManager.isInstanceOf[SortShuffleManager])) {
+ shuffleBlockPathResolver = shuffleManager.shuffleBlockManager.asInstanceOf[PathResolver]
+ true
+ } else {
+ false
+ }
+ }
// If we use Netty for shuffle, start a new Netty-based shuffle sender service.
private[storage] val nettyBlockClientFactory: BlockFetchingClientFactory = {
if (useNetty) new BlockFetchingClientFactory(conf) else null
}
+ /*
--- End diff --
u can remove this code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48574743
QA results for PR 1241:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16493/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53841508
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19454/consoleFull) for PR 1241 at commit [`0e01ae3`](https://github.com/apache/spark/commit/0e01ae321735aea5e4f568f247968e68e7f3ec33).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47314417
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52010865
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18417/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48561265
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16486/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14695968
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleManager.scala ---
@@ -49,8 +49,12 @@ private[spark] trait ShuffleManager {
endPartition: Int,
context: TaskContext): ShuffleReader[K, C]
- /** Remove a shuffle's metadata from the ShuffleManager. */
- def unregisterShuffle(shuffleId: Int)
+ /** Remove a shuffle's metadata from the ShuffleManager.
--- End diff --
use javadoc style rather than scaladoc here.
question: are we using the return value anywhere?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53950022
Merging this now. I will take care of some minor things myself. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53686699
Jenkins, test this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14696400
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -72,23 +74,48 @@ private[spark] class BlockManager(
// Actual storage of where blocks are kept
private var tachyonInitialized = false
private[storage] val memoryStore = new MemoryStore(this, maxMemory)
- private[storage] val diskStore = new DiskStore(this, diskBlockManager)
+ private[spark] val diskStore = new DiskStore(this, diskBlockManager)
private[storage] lazy val tachyonStore: TachyonStore = {
val storeDir = conf.get("spark.tachyonStore.baseDir", "/tmp_spark_tachyon")
val appFolderName = conf.get("spark.tachyonStore.folderName")
val tachyonStorePath = s"$storeDir/$appFolderName/${this.executorId}"
val tachyonMaster = conf.get("spark.tachyonStore.url", "tachyon://localhost:19998")
val tachyonBlockManager =
- new TachyonBlockManager(shuffleBlockManager, tachyonStorePath, tachyonMaster)
+ new TachyonBlockManager(this, tachyonStorePath, tachyonMaster)
tachyonInitialized = true
new TachyonStore(this, tachyonBlockManager)
}
+ val shuffleManager = {
--- End diff --
the problem here is that original shuffle Manager and blockManager's initial sequence is not defined. And they have multiple dependency , shuffleManager need to access some member from blockManager. And some of the BlockManager's member all access shuffleManager. Thus I make it work the same way as the ConnectionManager. If not init with blockManger, might need to enforce initial sequence in SparkEnv and might add extra code to make sure when they access each other. the instance in SparkEnv is already initiated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16928153
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockId.scala ---
@@ -59,6 +59,11 @@ case class ShuffleBlockId(shuffleId: Int, mapId: Int, reduceId: Int) extends Blo
}
@DeveloperApi
+case class ShuffleDataBlockId(shuffleId: Int, mapId: Int, reduceId: Int) extends BlockId {
--- End diff --
if the reduce id is always 0, why even bother defining it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52018311
QA results for PR 1241:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18435/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52018301
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18435/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47301718
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47495497
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16253/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759034
--- Diff: core/src/main/scala/org/apache/spark/shuffle/FileShuffleBlockManager.scala ---
@@ -15,21 +15,24 @@
* limitations under the License.
*/
-package org.apache.spark.storage
+package org.apache.spark.shuffle
import java.io.File
+import java.nio.ByteBuffer
import java.util.concurrent.ConcurrentLinkedQueue
import java.util.concurrent.atomic.AtomicInteger
import scala.collection.JavaConversions._
+import scala.Some
--- End diff --
remove this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47491606
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52146432
@mateiz rebased to latest master code. Have time to take a look?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48561263
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16759951
--- Diff: core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala ---
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.shuffle
+
+import java.io._
+import java.nio.ByteBuffer
+
+import org.apache.spark.{SparkEnv, SparkConf}
+import org.apache.spark.network.netty.PathResolver
+import org.apache.spark.serializer.Serializer
+import org.apache.spark.storage._
+import org.apache.spark.storage.ShuffleBlockId
+
+private[spark]
+class IndexShuffleBlockManager(conf: SparkConf)
+ extends ShuffleBlockManager with PathResolver {
+
+ private lazy val blockManager = SparkEnv.get.blockManager
+
+ // Mapping to a single shuffleBlockId with reduce ID 0 that we'll used to write all results to.
+ private def consolidatedId(blockId: ShuffleBlockId): ShuffleBlockId = {
+ blockId.copy(reduceId = 0)
+ }
+
+ def getDataFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId))
+ }
+
+ private def getIndexFile(blockId: ShuffleBlockId): File = {
+ blockManager.diskBlockManager.getFile(consolidatedId(blockId).name + ".index")
+ }
+
+ // Remove data file and index file that contain the output data from one map.
+ // Only need to be called once for all shuffleBlockId belong to one map output.
+ def removeDataByMap(blockId: ShuffleBlockId): Unit = {
+ var file = getDataFile(blockId)
+ if (file.exists) {
+ file.delete
+ }
+
+ file = getIndexFile(blockId)
+ if (file.exists) {
+ file.delete
+ }
+ }
+
+ // Write an index file with the offsets of each block, plus a final offset at the end for the
+ // end of the output file. This will be used by getBlockLocation to figure out where each block
+ // begins and ends.
+ def writeIndexFile(blockId: ShuffleBlockId, offsets: Array[Long]) = {
+ val indexFile = getIndexFile(blockId)
+ val out = new DataOutputStream(new BufferedOutputStream(new FileOutputStream(indexFile)))
+ try {
+ for(offset <- offsets) {
+ out.writeLong(offset)
+ }
+ } finally {
+ out.close()
+ }
+ }
+
+ /**
+ * Get the location of a block in a map output file. Uses the index file we create for it.
+ * */
+ def getBlockLocation(blockId: ShuffleBlockId): FileSegment = {
--- End diff --
since ShuffleBlockId is just a BlockId, can you make this private?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48558966
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16486/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47495607
Thanks - we are all super busy with Spark Summit this week so probably will get to this later in the week... feel free to send a reminder if I don't revisit this towards the end of the week.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48559404
@rxin addressed comments. Among them:
1. Rename HashShuffleBlockManager to FileShuffleBlockManager since it mainly handle block in File way. Though say sort based shuffle manager's own ShuffleBlockManager might also use File, but it might use another more specific name.
2. I still keep the shuffleManager inside blockManager, the main reason is that the netty staff need to access shuffleManager upon blockManager init, while in this time SparkEnv not fully created yet, you can not access shuffleManager though env.get approaching. Then you had to pass in shuffleManager to blockManager as a parameter which further complicate blockManager's interface. so to keep it simple, I think we might keep it this way until one day we did get rid of netty staff. And more, since the current BlockManager's logic depends on shuffleManager a lot, and there are no e.g. standalone "shuffleManagerMaster" things, so all shuffle data fetch works go through blockManager's interface firstly anyway, so it seems to me not totally unreasonable to put shuffleManager inside BlockManager?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53519186
Jenkins, test this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48558907
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by colorant <gi...@git.apache.org>.
Github user colorant commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r16760900
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -107,6 +153,11 @@ private[spark] class BlockManager(
}
}
+ if (useNetty && (shuffleManager.isInstanceOf[HashShuffleManager]
+ || shuffleManager.isInstanceOf[SortShuffleManager])) {
+ val resolver = shuffleManager.shuffleBlockManager.asInstanceOf[PathResolver]
--- End diff --
oh, I c, sorry for that. should do a code clean to remove the redundant code before push
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-47493566
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1241#discussion_r14278746
--- Diff: core/src/main/scala/org/apache/spark/shuffle/ShuffleManager.scala ---
@@ -50,7 +50,9 @@ private[spark] trait ShuffleManager {
context: TaskContext): ShuffleReader[K, C]
/** Remove a shuffle's metadata from the ShuffleManager. */
- def unregisterShuffle(shuffleId: Int)
+ def unregisterShuffle(shuffleId: Int): Boolean
--- End diff --
add @return explaining what the boolean return value means
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-48281236
Sorry will take a look tomorrow!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-52139906
QA tests have started for PR 1241. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18515/consoleFull
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2288] Hide ShuffleBlockManager behind S...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1241#issuecomment-53531388
Thanks for doing this. To help with the review, can you write a short design doc discussing the interfaces between different components, similar to the one attached here https://issues.apache.org/jira/browse/SPARK-3019 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org