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-3797] Minor addendum to Yarn shuffle service #3144

Closed
wants to merge 5 commits into from
Closed

[SPARK-3797] Minor addendum to Yarn shuffle service #3144

wants to merge 5 commits into from

Conversation

andrewor14
Copy link
Contributor

I did not realize there was a network.util.JavaUtils when I wrote this code. This PR moves the ByteBuffer string conversion to the appropriate place. I tested the changes on a stable yarn cluster.

@andrewor14 andrewor14 changed the title Move byte buffer String conversion logic to JavaUtils [SPARK-3797] Minor addendum to Yarn shuffle service Nov 6, 2014
@SparkQA
Copy link

SparkQA commented Nov 6, 2014

Test build #23021 has started for PR 3144 at commit adf186d.

  • This patch merges cleanly.

vanzin
vanzin reviewed Nov 6, 2014
View reviewed changes
yarn/alpha/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnable.scala
@@ -98,7 +98,9 @@ class ExecutorRunnable(
val secretString = securityMgr.getSecretKey()
val secretBytes =
if (secretString != null) {
ShuffleSecretManager.stringToBytes(secretString)
// This uses a JavaUtils method because the reverse conversion takes
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not sure the comment is useful. Might as well say it uses JavaUtils because that's where the method is implemented...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah maybe. It's just that it's not super intuitive why we need to use JavaUtils in scala code when we could have used something from Spark's Utils. I'd rather err on the safe side of being verbose. Maybe I'll reword the comment a little to make it sound less stupid.

@vanzin
Copy link
Contributor

vanzin commented Nov 6, 2014

LGTM. I guess the network code can't depend on Guava (so it could use Charsets.UTF_8)?

aarondav
aarondav reviewed Nov 6, 2014
View reviewed changes
network/common/src/main/java/org/apache/spark/network/util/JavaUtils.java
* converted back to the same string through {@link #bytesToString(ByteBuffer)}.
*/
public static ByteBuffer stringToBytes(String s) {
return ByteBuffer.wrap(s.getBytes(UTF8_CHARSET));
Copy link
Contributor

Choose a reason for hiding this comment

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

use Charsets.UTF_8

@SparkQA
Copy link

SparkQA commented Nov 6, 2014

Test build #23024 has started for PR 3144 at commit 94e205c.

  • This patch merges cleanly.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23023/
Test FAILed.

@andrewor14
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Nov 6, 2014

Test build #23025 has started for PR 3144 at commit 94e205c.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Nov 6, 2014

Test build #23021 has finished for PR 3144 at commit adf186d.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23021/
Test PASSed.

@andrewor14
Copy link
Contributor Author

Ok I'm merging this thanks for the comments @aarondav and @vanzin

@andrewor14
Copy link
Contributor Author

Oh wait JK the tests that just passed are not from the recent changes. I'll wait a little longer.

@SparkQA
Copy link

SparkQA commented Nov 7, 2014

Test build #23030 has started for PR 3144 at commit b6c08bf.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Nov 7, 2014

Test build #23024 has finished for PR 3144 at commit 94e205c.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23024/
Test PASSed.

@aarondav
Copy link
Contributor

aarondav commented Nov 7, 2014

LGTM

@SparkQA
Copy link

SparkQA commented Nov 7, 2014

Test build #23025 has finished for PR 3144 at commit 94e205c.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23025/
Test PASSed.

@andrewor14
Copy link
Contributor Author

Ok merging for real

@asfgit asfgit closed this in 96136f2 Nov 7, 2014
asfgit pushed a commit that referenced this pull request Nov 7, 2014
[SPARK-3797] Minor addendum to Yarn shuffle service
6508953 
I did not realize there was a `network.util.JavaUtils` when I wrote this code. This PR moves the `ByteBuffer` string conversion to the appropriate place. I tested the changes on a stable yarn cluster.

Author: Andrew Or <andrew@databricks.com>

Closes #3144 from andrewor14/yarn-shuffle-util and squashes the following commits:

b6c08bf [Andrew Or] Remove unused import
94e205c [Andrew Or] Use netty Unpooled
85202a5 [Andrew Or] Use guava Charsets
057135b [Andrew Or] Reword comment
adf186d [Andrew Or] Move byte buffer String conversion logic to JavaUtils

(cherry picked from commit 96136f2)
Signed-off-by: Andrew Or <andrew@databricks.com>
@andrewor14 andrewor14 deleted the yarn-shuffle-util branch November 7, 2014 01:21
@SparkQA
Copy link

SparkQA commented Nov 7, 2014

Test build #23030 has finished for PR 3144 at commit b6c08bf.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23030/
Test PASSed.

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.
5 participants
@andrewor14 @SparkQA @vanzin @AmplabJenkins @aarondav