Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[SPARK-21369][Core]Don't use Scala Tuple2 in common/network-* #18593

Closed
wants to merge 1 commit into from

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Jul 10, 2017

What changes were proposed in this pull request?

Remove all usages of Scala Tuple2 from common/network-* projects. Otherwise, Yarn users cannot use spark.reducer.maxReqSizeShuffleToMem.

How was this patch tested?

Jenkins.

@zsxwing zsxwing changed the title [SPARK-21369][Core]Don't use Scala Tuple in common/network-* [SPARK-21369][Core]Don't use Scala Tuple2 in common/network-* Jul 10, 2017
@@ -90,7 +90,8 @@
<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-tags_${scala.binary.version}</artifactId>
</dependency>
<scope>test</scope>
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to not add Scala library into the compile scope.

return String.format("%d_%d", streamId, chunkId);
}

public static Tuple2<Long, Integer> parseStreamChunkId(String streamChunkId) {
Copy link
Member Author

Choose a reason for hiding this comment

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

the change in this file is just inlining this method.

@zsxwing
Copy link
Member Author

zsxwing commented Jul 10, 2017

cc @cloud-fan @jinxing64

@vanzin
Copy link
Contributor

vanzin commented Jul 10, 2017

LGTM.

@vanzin
Copy link
Contributor

vanzin commented Jul 10, 2017

Also, an alternative would be to explicitly add the scala-library dependency in test scope, in case some future dependency re-adds it in compile scope.

@zsxwing
Copy link
Member Author

zsxwing commented Jul 10, 2017

@vanzin it doesn't work. I added scala-library into the test scope and changed org.apache.spark:spark-tags back to the compile scope, but the build didn't fail.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM, too.

@SparkQA
Copy link

SparkQA commented Jul 10, 2017

Test build #79479 has finished for PR 18593 at commit 3d1a6ff.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

@jinxing64
Copy link

LGTM

@cloud-fan
Copy link
Contributor

LGTM, merging to master/2.2!

asfgit pushed a commit that referenced this pull request Jul 11, 2017
## What changes were proposed in this pull request?

Remove all usages of Scala Tuple2 from common/network-* projects. Otherwise, Yarn users cannot use `spark.reducer.maxReqSizeShuffleToMem`.

## How was this patch tested?

Jenkins.

Author: Shixiong Zhu <shixiong@databricks.com>

Closes #18593 from zsxwing/SPARK-21369.

(cherry picked from commit 833eab2)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@asfgit asfgit closed this in 833eab2 Jul 11, 2017
@zsxwing zsxwing deleted the SPARK-21369 branch July 11, 2017 22:06
MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
## What changes were proposed in this pull request?

Remove all usages of Scala Tuple2 from common/network-* projects. Otherwise, Yarn users cannot use `spark.reducer.maxReqSizeShuffleToMem`.

## How was this patch tested?

Jenkins.

Author: Shixiong Zhu <shixiong@databricks.com>

Closes apache#18593 from zsxwing/SPARK-21369.

(cherry picked from commit 833eab2)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants