You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by liyezhang556520 <gi...@git.apache.org> on 2014/10/14 07:46:32 UTC
[GitHub] spark pull request: [SPARK-3941][CORE] _remainingmem should not in...
GitHub user liyezhang556520 opened a pull request:
https://github.com/apache/spark/pull/2792
[SPARK-3941][CORE] _remainingmem should not increase twice when updateBlockInfo
In BlockManagermasterActor, _remainingMem would increase memSize for twice when updateBlockInfo if new storageLevel is invalid and old storageLevel is "useMemory". Also, _remainingMem should increase with original memory size instead of new memSize.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/liyezhang556520/spark spark-3941-remainMem
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2792.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 #2792
----
commit 0380a327a7d657891fcc42883504945eef2a758d
Author: Zhang, Liye <li...@intel.com>
Date: 2014-10-14T05:41:25Z
[SPARK-3941][CORE] _remainingmem should not increase twice when updateBlockInfo
----
---
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-3941][CORE] _remainingmem should not in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59007665
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21721/
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-3941][CORE] _remainingmem should not in...
Posted by liyezhang556520 <gi...@git.apache.org>.
Github user liyezhang556520 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2792#discussion_r18997684
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala ---
@@ -457,16 +457,18 @@ private[spark] class BlockManagerInfo(
if (_blocks.containsKey(blockId)) {
// The block exists on the slave already.
- val originalLevel: StorageLevel = _blocks.get(blockId).storageLevel
+ val blockStatus: BlockStatus = _blocks.get(blockId)
+ val originalLevel: StorageLevel = blockStatus.storageLevel
+ val originalMemSize: Long = blockStatus.memSize
if (originalLevel.useMemory) {
- _remainingMem += memSize
+ _remainingMem += originalMemSize
--- End diff --
"memSize" is the current memory that the block consume, "originalMemSize" is the memory used before block updating. These two size might be not the same. So here, if the block exists on the slave already, `_remainingmem` should first `_remainingMem += originalMemSize` and then `_remainingMem -= memSize` if the new storage level is "useMemory". Do I have misunderstanding of these two mem size?
---
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-3941][CORE] _remainingmem should not in...
Posted by liyezhang556520 <gi...@git.apache.org>.
Github user liyezhang556520 commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59452527
Good idea, I'll update it soon.
---
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-3941][CORE] _remainingmem should not in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59454244
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21829/consoleFull) for PR 2792 at commit [`3d487cc`](https://github.com/apache/spark/commit/3d487cc1bdae1e373b5673fb52ebc6709a592256).
* This patch merges cleanly.
---
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-3941][CORE] _remainingmem should not in...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59451980
Hey @liyezhang556520 I believe your fix is correct. However, if we put it in the `isValid` case it seems that it's now duplicated in two places. Would it make sense to keep the code you deleted and remove it in the case where it's `inValid`?
---
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-3941][CORE] _remainingmem should not in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-58994396
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21717/
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-3941][CORE] _remainingmem should not in...
Posted by liyezhang556520 <gi...@git.apache.org>.
Github user liyezhang556520 commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59453848
@andrewor14 , code updated, would you please have a look?
---
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-3941][CORE] _remainingmem should not in...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2792#discussion_r18995941
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala ---
@@ -455,18 +455,21 @@ private[spark] class BlockManagerInfo(
updateLastSeenMs()
- if (_blocks.containsKey(blockId)) {
- // The block exists on the slave already.
- val originalLevel: StorageLevel = _blocks.get(blockId).storageLevel
-
- if (originalLevel.useMemory) {
- _remainingMem += memSize
- }
- }
-
if (storageLevel.isValid) {
- /* isValid means it is either stored in-memory, on-disk or on-Tachyon.
- * But the memSize here indicates the data size in or dropped from memory,
+ // isValid means it is either stored in-memory, on-disk or on-Tachyon.
+
+ if (_blocks.containsKey(blockId)) {
+ // The block exists on the slave already.
+ val blockStatus: BlockStatus = _blocks.get(blockId)
+ val originalLevel: StorageLevel = blockStatus.storageLevel
+ val originalMemSize: Long = blockStatus.memSize
--- End diff --
The old code seems a little more concise
---
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-3941][CORE] _remainingmem should not in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59458375
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21829/
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-3941][CORE] _remainingmem should not in...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2792#discussion_r18997247
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala ---
@@ -457,16 +457,18 @@ private[spark] class BlockManagerInfo(
if (_blocks.containsKey(blockId)) {
// The block exists on the slave already.
- val originalLevel: StorageLevel = _blocks.get(blockId).storageLevel
+ val blockStatus: BlockStatus = _blocks.get(blockId)
+ val originalLevel: StorageLevel = blockStatus.storageLevel
+ val originalMemSize: Long = blockStatus.memSize
if (originalLevel.useMemory) {
- _remainingMem += memSize
+ _remainingMem += originalMemSize
--- End diff --
This seems like a change in semantics. Can you explain why the old code is incorrect? I'm just trying to understand what this block is trying to do.
---
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-3941][CORE] _remainingmem should not in...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2792#discussion_r18998452
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterActor.scala ---
@@ -457,16 +457,18 @@ private[spark] class BlockManagerInfo(
if (_blocks.containsKey(blockId)) {
// The block exists on the slave already.
- val originalLevel: StorageLevel = _blocks.get(blockId).storageLevel
+ val blockStatus: BlockStatus = _blocks.get(blockId)
+ val originalLevel: StorageLevel = blockStatus.storageLevel
+ val originalMemSize: Long = blockStatus.memSize
if (originalLevel.useMemory) {
- _remainingMem += memSize
+ _remainingMem += originalMemSize
--- End diff --
I see, we want to release the `originalMemSize` and occupy the new `memSize`. That makes sense. It seems that the old code is just wrong then.
---
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-3941][CORE] _remainingmem should not in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59458368
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21829/consoleFull) for PR 2792 at commit [`3d487cc`](https://github.com/apache/spark/commit/3d487cc1bdae1e373b5673fb52ebc6709a592256).
* 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-3941][CORE] _remainingmem should not in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59007655
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21721/consoleFull) for PR 2792 at commit [`0380a32`](https://github.com/apache/spark/commit/0380a327a7d657891fcc42883504945eef2a758d).
* 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-3941][CORE] _remainingmem should not in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-58999993
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21721/consoleFull) for PR 2792 at commit [`0380a32`](https://github.com/apache/spark/commit/0380a327a7d657891fcc42883504945eef2a758d).
* This patch merges cleanly.
---
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-3941][CORE] _remainingmem should not in...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-59457290
LGTM. I'm merging this.
---
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-3941][CORE] _remainingmem should not in...
Posted by liyezhang556520 <gi...@git.apache.org>.
Github user liyezhang556520 commented on the pull request:
https://github.com/apache/spark/pull/2792#issuecomment-58999763
Jenkins, retest this please.
---
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-3941][CORE] _remainingmem should not in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/2792
---
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