You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by dhruve <gi...@git.apache.org> on 2017/06/23 14:22:06 UTC
[GitHub] spark pull request #18407: [SPARK-21181] Release byteBuffers to suppress net...
GitHub user dhruve opened a pull request:
https://github.com/apache/spark/pull/18407
[SPARK-21181] Release byteBuffers to suppress netty error messages
## What changes were proposed in this pull request?
We are explicitly calling release on the byteBuf's used to encode the string to Base64 to suppress the memory leak error message reported by netty. This is to make it less confusing for the user.
### Changes proposed in this fix
By explicitly invoking release on the byteBuf's we are decrement the internal reference counts for the wrappedByteBuf's. Now, when the GC kicks in, these would be reclaimed as before, just that netty wouldn't report any memory leak error messages as the internal ref. counts are now 0.
## How was this patch tested?
Ran a few spark-applications and examined the logs. The error message no longer appears.
Original PR was opened against branch-2.1 => https://github.com/apache/spark/pull/18392
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dhruve/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18407.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 #18407
----
commit 51967c87360c3c78a5b0f7271c8bea017058911d
Author: Dhruve Ashar <dh...@gmail.com>
Date: 2017-06-22T16:37:55Z
[SPARK-21181] Release byteBuffers to suppress netty error messages
----
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18407
**[Test build #78532 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78532/testReport)** for PR 18407 at commit [`51967c8`](https://github.com/apache/spark/commit/51967c87360c3c78a5b0f7271c8bea017058911d).
* This patch passes all 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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by dhruve <gi...@git.apache.org>.
Github user dhruve commented on the issue:
https://github.com/apache/spark/pull/18407
Thanks @vanzin
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18407
**[Test build #78534 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78534/testReport)** for PR 18407 at commit [`d0ced9f`](https://github.com/apache/spark/commit/d0ced9f132752cdb2e5238cb1081c86964cff129).
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/18407
LGTM. Merging to master / 2.2 / 2.1.
---
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 #18407: [SPARK-21181] Release byteBuffers to suppress net...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18407
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18407
Merged build finished. Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18407: [SPARK-21181] Release byteBuffers to suppress net...
Posted by dhruve <gi...@git.apache.org>.
Github user dhruve commented on a diff in the pull request:
https://github.com/apache/spark/pull/18407#discussion_r123765067
--- Diff: common/network-common/src/main/java/org/apache/spark/network/sasl/SparkSaslServer.java ---
@@ -187,14 +188,30 @@ public void handle(Callback[] callbacks) throws IOException, UnsupportedCallback
/* Encode a byte[] identifier as a Base64-encoded string. */
public static String encodeIdentifier(String identifier) {
Preconditions.checkNotNull(identifier, "User cannot be null if SASL is enabled");
- return Base64.encode(Unpooled.wrappedBuffer(identifier.getBytes(StandardCharsets.UTF_8)))
- .toString(StandardCharsets.UTF_8);
+ return getBase64EncodedString(identifier);
}
/** Encode a password as a base64-encoded char[] array. */
public static char[] encodePassword(String password) {
Preconditions.checkNotNull(password, "Password cannot be null if SASL is enabled");
- return Base64.encode(Unpooled.wrappedBuffer(password.getBytes(StandardCharsets.UTF_8)))
- .toString(StandardCharsets.UTF_8).toCharArray();
+ return getBase64EncodedString(password).toCharArray();
+ }
+
+ /** Return a Base64-encoded string. */
+ private static String getBase64EncodedString (String str) {
+ ByteBuf byteBuf = null, encodedByteBuf = null;
--- End diff --
Okay. The order of releasing the buffers does not matter. We check for the byteBuf first just incase if we hit an exception while trying to acquire it, we know we don't need to check encodedByteBuf.
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18407
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78534/
Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18407
**[Test build #78532 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78532/testReport)** for PR 18407 at commit [`51967c8`](https://github.com/apache/spark/commit/51967c87360c3c78a5b0f7271c8bea017058911d).
---
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 #18407: [SPARK-21181] Release byteBuffers to suppress net...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/18407#discussion_r123760811
--- Diff: common/network-common/src/main/java/org/apache/spark/network/sasl/SparkSaslServer.java ---
@@ -187,14 +188,30 @@ public void handle(Callback[] callbacks) throws IOException, UnsupportedCallback
/* Encode a byte[] identifier as a Base64-encoded string. */
public static String encodeIdentifier(String identifier) {
Preconditions.checkNotNull(identifier, "User cannot be null if SASL is enabled");
- return Base64.encode(Unpooled.wrappedBuffer(identifier.getBytes(StandardCharsets.UTF_8)))
- .toString(StandardCharsets.UTF_8);
+ return getBase64EncodedString(identifier);
}
/** Encode a password as a base64-encoded char[] array. */
public static char[] encodePassword(String password) {
Preconditions.checkNotNull(password, "Password cannot be null if SASL is enabled");
- return Base64.encode(Unpooled.wrappedBuffer(password.getBytes(StandardCharsets.UTF_8)))
- .toString(StandardCharsets.UTF_8).toCharArray();
+ return getBase64EncodedString(password).toCharArray();
+ }
+
+ /** Return a Base64-encoded string. */
+ private static String getBase64EncodedString (String str) {
+ ByteBuf byteBuf = null, encodedByteBuf = null;
--- End diff --
Normally these would be on two lines -- think the linter will complain about this (but not fail the build, so take a look).
Also does it matter that they're not released in the reverse order they were obtained?
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18407
Merged build finished. Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18407: [SPARK-21181] Release byteBuffers to suppress net...
Posted by dhruve <gi...@git.apache.org>.
Github user dhruve commented on a diff in the pull request:
https://github.com/apache/spark/pull/18407#discussion_r123773915
--- Diff: common/network-common/src/main/java/org/apache/spark/network/sasl/SparkSaslServer.java ---
@@ -187,14 +188,30 @@ public void handle(Callback[] callbacks) throws IOException, UnsupportedCallback
/* Encode a byte[] identifier as a Base64-encoded string. */
public static String encodeIdentifier(String identifier) {
Preconditions.checkNotNull(identifier, "User cannot be null if SASL is enabled");
- return Base64.encode(Unpooled.wrappedBuffer(identifier.getBytes(StandardCharsets.UTF_8)))
- .toString(StandardCharsets.UTF_8);
+ return getBase64EncodedString(identifier);
}
/** Encode a password as a base64-encoded char[] array. */
public static char[] encodePassword(String password) {
Preconditions.checkNotNull(password, "Password cannot be null if SASL is enabled");
- return Base64.encode(Unpooled.wrappedBuffer(password.getBytes(StandardCharsets.UTF_8)))
- .toString(StandardCharsets.UTF_8).toCharArray();
+ return getBase64EncodedString(password).toCharArray();
+ }
+
+ /** Return a Base64-encoded string. */
+ private static String getBase64EncodedString (String str) {
+ ByteBuf byteBuf = null, encodedByteBuf = null;
--- End diff --
Had to fix a space, refactored the code. The linter however didn't complain.
---
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 issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18407
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78532/
Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18407: [SPARK-21181] Release byteBuffers to suppress netty erro...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18407
**[Test build #78534 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78534/testReport)** for PR 18407 at commit [`d0ced9f`](https://github.com/apache/spark/commit/d0ced9f132752cdb2e5238cb1081c86964cff129).
* This patch passes all 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