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.

Sign up for GitHub

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

Jump to bottom

[SPARK-2897][SPARK-2920]TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer" #1836

Closed
wants to merge 3 commits into from
Closed

[SPARK-2897][SPARK-2920]TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer" #1836

wants to merge 3 commits into from

Conversation

witgo
Copy link
Contributor

@witgo witgo commented Aug 7, 2014

No description provided.

@SparkQA
Copy link

SparkQA commented Aug 7, 2014

QA tests have started for PR 1836. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18121/consoleFull

@SparkQA
Copy link

SparkQA commented Aug 7, 2014

QA results for PR 1836:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18121/consoleFull

@witgo witgo changed the title [SPARK-2897]org.apache.spark.broadcast.TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer" [SPARK-2897][WIP]TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer" Aug 8, 2014
@witgo witgo changed the title [SPARK-2897][WIP]TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer" [SPARK-2897][SPARK-2920]TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer" Aug 8, 2014
@SparkQA
Copy link

SparkQA commented Aug 8, 2014

QA tests have started for PR 1836. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18180/consoleFull

@SparkQA
Copy link

SparkQA commented Aug 8, 2014

QA results for PR 1836:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18180/consoleFull

rxin
rxin reviewed Aug 8, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
def blockifyObject[T](obj: T): TorrentInfo = {
val byteArray = Utils.serialize[T](obj)
def blockifyObject[T: ClassTag](obj: T): TorrentInfo = {
val bos =new ByteArrayOutputStream()
Copy link
Contributor

Choose a reason for hiding this comment

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

can u add a space after =?

@rxin
Copy link
Contributor

rxin commented Aug 8, 2014

This looks good to me other than the minor style problems I pointed out. Thanks for doing it.

markhamstra
markhamstra reviewed Aug 8, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
@@ -17,14 +17,14 @@

package org.apache.spark.broadcast

import java.io.{ByteArrayInputStream, ObjectInputStream, ObjectOutputStream}
import java.io._
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer that the imports remain explicit when there are this few.

@rxin It doesn't look like we have a style policy for when it is okay to use wildcard imports?

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't, but I agree with you.

@rxin
Copy link
Contributor

rxin commented Aug 8, 2014

Can we also add a unit test for these two?

@SparkQA
Copy link

SparkQA commented Aug 8, 2014

QA tests have started for PR 1836. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18222/consoleFull

witgo
witgo reviewed Aug 8, 2014
View reviewed changes
core/src/test/scala/org/apache/spark/broadcast/BroadcastSuite.scala
@@ -69,6 +71,8 @@ class BroadcastSuite extends FunSuite with LocalSparkContext {

test("Accessing TorrentBroadcast variables in a local cluster") {
val numSlaves = 4
torrentConf.set("spark.serializer", "org.apache.spark.serializer.KryoSerializer")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be no need to add new test.

@SparkQA
Copy link

SparkQA commented Aug 8, 2014

QA tests have started for PR 1836. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18225/consoleFull

@SparkQA
Copy link

SparkQA commented Aug 8, 2014

QA results for PR 1836:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18222/consoleFull

@SparkQA
Copy link

SparkQA commented Aug 8, 2014

QA results for PR 1836:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18225/consoleFull

@rxin
Copy link
Contributor

rxin commented Aug 8, 2014

Thanks. Merging this in master and branch-1.1 and branch-1.0.

asfgit pushed a commit that referenced this pull request Aug 8, 2014
@witgo @rxin
[SPARK-2897][SPARK-2920]TorrentBroadcast does use the serializer clas…
bae35b1 
…s specified in the spark option "spark.serializer"

Author: GuoQiang Li <[email protected]>

Closes #1836 from witgo/SPARK-2897 and squashes the following commits:

23cdc5b [GuoQiang Li] review commit
ada4fba [GuoQiang Li] TorrentBroadcast does not support broadcast compression
fb91792 [GuoQiang Li] org.apache.spark.broadcast.TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer"

(cherry picked from commit ec79063)
Signed-off-by: Reynold Xin <[email protected]>
@asfgit asfgit closed this in ec79063 Aug 8, 2014
asfgit pushed a commit that referenced this pull request Aug 8, 2014
@witgo @rxin
[SPARK-2897][SPARK-2920]TorrentBroadcast does use the serializer clas…
dd11e4e 
…s specified in the spark option "spark.serializer"

Author: GuoQiang Li <[email protected]>

Closes #1836 from witgo/SPARK-2897 and squashes the following commits:

23cdc5b [GuoQiang Li] review commit
ada4fba [GuoQiang Li] TorrentBroadcast does not support broadcast compression
fb91792 [GuoQiang Li] org.apache.spark.broadcast.TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer"

(cherry picked from commit ec79063)
Signed-off-by: Reynold Xin <[email protected]>
@witgo witgo deleted the SPARK-2897 branch August 9, 2014 01:08
xiliu82 pushed a commit to xiliu82/spark that referenced this pull request Sep 4, 2014
@witgo @conviva-zz
[SPARK-2897][SPARK-2920]TorrentBroadcast does use the serializer clas…
1f20374 
…s specified in the spark option "spark.serializer"

Author: GuoQiang Li <[email protected]>

Closes apache#1836 from witgo/SPARK-2897 and squashes the following commits:

23cdc5b [GuoQiang Li] review commit
ada4fba [GuoQiang Li] TorrentBroadcast does not support broadcast compression
fb91792 [GuoQiang Li] org.apache.spark.broadcast.TorrentBroadcast does use the serializer class specified in the spark option "spark.serializer"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@witgo @SparkQA @rxin @markhamstra