Skip to content

[SPARK-11563] [core] [repl] Use RpcEnv to transfer REPL-generated classes. #9923

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
Closed
wants to merge 5 commits into from
Closed

[SPARK-11563] [core] [repl] Use RpcEnv to transfer REPL-generated classes. #9923

wants to merge 5 commits into from

Conversation

@vanzin
Copy link
Contributor

@vanzin vanzin commented Nov 24, 2015

This avoids bringing up yet another HTTP server on the driver, and
instead reuses the file server already managed by the driver's
RpcEnv. As a bonus, the repl now inherits the security features of
the network library.

There's also a small change to create the directory for storing classes
under the root temp dir for the application (instead of directly
under java.io.tmpdir).

[SPARK-11563] [core] [repl] Use RpcEnv to transfer REPL-generated cla…
ddd1624 
…sses.

This avoids bringing up yet another HTTP server on the driver, and
instead reuses the file server already managed by the driver's
RpcEnv. As a bonus, the repl now inherits the security features of
the network library.

A few small fixes were made to bugs uncovered by these changes:
- NettyRpcEnv::openStream() now correctly propagates errors to
  the read side of the pipe.
- NettyStreamManager now throws if the file being transferred does
  not exist
- The network library now correctly handles zero-sized streams.
@vanzin
Copy link
Contributor Author

vanzin commented Nov 24, 2015

/cc @rxin @zsxwing

@SparkQA
Copy link

SparkQA commented Nov 24, 2015

Test build #46566 has finished for PR 9923 at commit ddd1624.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * class ExecutorClassLoader(\n

@squito squito mentioned this pull request Nov 25, 2015
Closed
@vanzin
Copy link
Contributor Author

vanzin commented Nov 25, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Nov 26, 2015

Test build #46710 has finished for PR 9923 at commit e579f53.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * class ExecutorClassLoader(\n

@vanzin
Copy link
Contributor Author

vanzin commented Nov 26, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Nov 26, 2015

Test build #46731 has finished for PR 9923 at commit e579f53.

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

squito
squito reviewed Dec 3, 2015
View reviewed changes
repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala Outdated
Copy link
Contributor

@squito squito Dec 3, 2015

Choose a reason for hiding this comment

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

super nit: RpcEnvFileServer is unused

@squito
Copy link
Contributor

squito commented Dec 3, 2015

looks reasonable to me, I tested out a repl with both netty & akka in local-cluster mode as well. Would appreciate another set of eyes on it.

@SparkQA
Copy link

SparkQA commented Dec 3, 2015

Test build #47155 has finished for PR 9923 at commit e5c1973.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * class ExecutorClassLoader(\n

@vanzin
Copy link
Contributor Author

vanzin commented Dec 7, 2015

Given the silence I guess people are ok with the patch, so I'll push this soon. Meanwhile, retest this please.

rxin
rxin reviewed Dec 8, 2015
View reviewed changes
repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala Outdated
Copy link
Contributor

@rxin rxin Dec 8, 2015

Choose a reason for hiding this comment

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

getClassFileInputStreamFromSpark -> getClassFileInputStreamFromSparkRPC

@rxin
Copy link
Contributor

rxin commented Dec 8, 2015

IIUC, http server is useless once we remove akka? Can you add a line of comment there as well?

LGTM.

@vanzin
Copy link
Contributor Author

vanzin commented Dec 8, 2015

IIUC, http server is useless once we remove akka?

There's the http-based broadcast server, which IIRC is not the default. Does anyone even use that these days?

@SparkQA
Copy link

SparkQA commented Dec 8, 2015

Test build #47354 has finished for PR 9923 at commit 09d03e7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * class ExecutorClassLoader(\n

@SparkQA
Copy link

SparkQA commented Dec 10, 2015

Test build #47525 has finished for PR 9923 at commit 08a74e5.

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

@vanzin
Copy link
Contributor Author

vanzin commented Dec 10, 2015

Merging to master.

@asfgit asfgit closed this in 4a46b88 Dec 10, 2015
@vanzin vanzin deleted the SPARK-11563 branch December 30, 2015 23:28
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

@vanzin @SparkQA @squito @rxin