You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by zsxwing <gi...@git.apache.org> on 2015/12/01 22:44:54 UTC
[GitHub] spark pull request: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
GitHub user zsxwing opened a pull request:
https://github.com/apache/spark/pull/10076
[SPARK-12078][Core]Fix ByteBuffer.limit misuse
`ByteBuffer.limit` is not the remaining size of ByteBuffer. `ByteBuffer.limit` is equal to `ByteBuffer.remaining` only if `ByteBuffer.position` is 0.
I just went through the codes and replaced misused `limit` with `remaining`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zsxwing/spark bytebuffer-limit
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10076.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 #10076
----
commit 2073aeae35ee2fd6de5f3e34238129be98ff7c8f
Author: Shixiong Zhu <sh...@databricks.com>
Date: 2015-12-01T21:09:35Z
Fix ByteBuffer.limit misuse
----
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162735915
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46395903
--- Diff: core/src/main/scala/org/apache/spark/storage/ExternalBlockStore.scala ---
@@ -110,7 +110,7 @@ private[spark] class ExternalBlockStore(blockManager: BlockManager, executorId:
val byteBuffer = bytes.duplicate()
byteBuffer.rewind()
externalBlockManager.get.putBytes(blockId, byteBuffer)
- val size = bytes.limit()
+ val size = bytes.remaining()
--- End diff --
This is an interesting one. The original `bytes` is reset to position 0 and put, so it means that the size that was put really is `bytes.limit` or `byteBuffer.remaining` (before it's put). I think this might have to be adjusted but I'm also not sure why `bytes` is returned here instead of what was put into the block manager. Maybe it's really assumed position is 0, but then why `rewind`?
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-161107268
**[Test build #46995 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/46995/consoleFull)** for PR 10076 at commit [`2073aea`](https://github.com/apache/spark/commit/2073aeae35ee2fd6de5f3e34238129be98ff7c8f).
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162735917
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47291/
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-161387484
I want to put this one on hold until #10083 gets merged.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-161138536
Merged build finished. Test FAILed.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-161138537
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/46995/
Test FAILed.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46346022
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -253,7 +253,7 @@ private[spark] class Executor(
val directResult = new DirectTaskResult(valueBytes, accumUpdates, task.metrics.orNull)
val serializedDirectResult = ser.serialize(directResult)
- val resultSize = serializedDirectResult.limit
+ val resultSize = serializedDirectResult.remaining()
--- End diff --
You're right that there's an implicit assumption in some of this code that the buffer's position is 0 on returning, and the entire buffer is filled with valid data. Do we have a situation where the position is not 0 though, but is correctly at the start of the data? at least, this looks like it handles the situation, but it sounds unusual. Equally, if that's an issue, are we sure the entire buffer has valid data, through the end? that assumption is still present here, that the end of the data is the end of the buffer.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162646134
**[Test build #47281 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47281/consoleFull)** for PR 10076 at commit [`5d5ab7e`](https://github.com/apache/spark/commit/5d5ab7ef3c49a2a47c0521318be20c0e8f5904a3).
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162686192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47281/
Test FAILed.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162686073
**[Test build #47281 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47281/consoleFull)** for PR 10076 at commit [`5d5ab7e`](https://github.com/apache/spark/commit/5d5ab7ef3c49a2a47c0521318be20c0e8f5904a3).
* This patch **fails Spark unit 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 pull request: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46396036
--- Diff: core/src/main/scala/org/apache/spark/storage/MemoryStore.scala ---
@@ -96,8 +96,8 @@ private[spark] class MemoryStore(blockManager: BlockManager, memoryManager: Memo
putIterator(blockId, values, level, returnValues = true)
} else {
val droppedBlocks = new ArrayBuffer[(BlockId, BlockStatus)]
- tryToPut(blockId, bytes, bytes.limit, deserialized = false, droppedBlocks)
- PutResult(bytes.limit(), Right(bytes.duplicate()), droppedBlocks)
+ tryToPut(blockId, bytes, bytes.remaining(), deserialized = false, droppedBlocks)
--- End diff --
Same, `remaining` changes here?
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162735812
**[Test build #47291 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47291/consoleFull)** for PR 10076 at commit [`4cdbc7c`](https://github.com/apache/spark/commit/4cdbc7c06d29abc8a0df40979b2ba05566354ac6).
* 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 pull request: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-161138492
**[Test build #46995 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/46995/consoleFull)** for PR 10076 at commit [`2073aea`](https://github.com/apache/spark/commit/2073aeae35ee2fd6de5f3e34238129be98ff7c8f).
* This patch **fails Spark unit 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 pull request: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46394928
--- Diff: core/src/main/scala/org/apache/spark/scheduler/TaskResultGetter.scala ---
@@ -51,14 +51,14 @@ private[spark] class TaskResultGetter(sparkEnv: SparkEnv, scheduler: TaskSchedul
try {
val (result, size) = serializer.get().deserialize[TaskResult[_]](serializedData) match {
case directResult: DirectTaskResult[_] =>
- if (!taskSetManager.canFetchMoreResults(serializedData.limit())) {
+ if (!taskSetManager.canFetchMoreResults(serializedData.remaining())) {
--- End diff --
This looks like it makes a unit test fail. I think you may have to check the size before the deserializer consumes the byte buffer?
This is overall looking good but we probably have to comb through these a little more to think through the implications.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162710108
**[Test build #47291 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47291/consoleFull)** for PR 10076 at commit [`4cdbc7c`](https://github.com/apache/spark/commit/4cdbc7c06d29abc8a0df40979b2ba05566354ac6).
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10076#issuecomment-162686186
Merged build finished. Test FAILed.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46395247
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala ---
@@ -607,7 +607,7 @@ private[spark] class BlockManager(
return Some(new BlockResult(
dataDeserialize(blockId, data),
DataReadMethod.Network,
- data.limit()))
+ data.remaining()))
--- End diff --
Same here -- does `dataDeserialize` not consume the buffer?
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46992341
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -253,7 +253,7 @@ private[spark] class Executor(
val directResult = new DirectTaskResult(valueBytes, accumUpdates, task.metrics.orNull)
val serializedDirectResult = ser.serialize(directResult)
- val resultSize = serializedDirectResult.limit
+ val resultSize = serializedDirectResult.remaining()
--- End diff --
I found I was wrong about the position of `ByteBuffer` from Netty. Netty will call `ByteBuffer.slice` to reset the position to 0 before returning it: https://github.com/netty/netty/blob/0f9492c9affc528c766f9677952412564d4a3f6d/buffer/src/main/java/io/netty/buffer/PooledHeapByteBuf.java#L269
I think we don't need this patch.
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing closed the pull request at:
https://github.com/apache/spark/pull/10076
---
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: [SPARK-12078][Core]Fix ByteBuffer.limit misuse
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/10076#discussion_r46364753
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -253,7 +253,7 @@ private[spark] class Executor(
val directResult = new DirectTaskResult(valueBytes, accumUpdates, task.metrics.orNull)
val serializedDirectResult = ser.serialize(directResult)
- val resultSize = serializedDirectResult.limit
+ val resultSize = serializedDirectResult.remaining()
--- End diff --
> Do we have a situation where the position is not 0 though, but is correctly at the start of the data?
If a `ByteBuffer` is from Netty, the position could be a non-zero value.
> Equally, if that's an issue, are we sure the entire buffer has valid data, through the end? that assumption is still present here, that the end of the data is the end of the buffer.
The `ByteBuffer` may contain more data internally, but the user should only read the part between `position` and `limit`. I think that's defined in `ByteBuffer/Buffer` javadoc.
---
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