You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jianran <gi...@git.apache.org> on 2017/03/16 10:41:06 UTC
[GitHub] spark pull request #17313: [SPARK-19974][Block Manager] in-memory LRU for pa...
GitHub user jianran opened a pull request:
https://github.com/apache/spark/pull/17313
[SPARK-19974][Block Manager] in-memory LRU for partitions of multiple RDDs testcase did a get that is no sense
## What changes were proposed in this pull request?
in-memory LRU for partitions of multiple RDDs testcase did a get that is no sense; rdd_0_2 is the most recently used item that isn't the reason the rdd_0_2 is remain; `store.getSingleAndReleaseLock(rdd(0, 2)).isDefined ` as same as the previous line `store.memoryStore.contains(rdd(0, 2))`, so remove the no sense code
## How was this patch tested?
run the test
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jianran/spark samerddrule
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17313.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 #17313
----
commit 2710de064ee3cad8c5db117f8fd4b1a984303b1e
Author: jianran.tfh <ji...@taobao.com>
Date: 2017-03-16T07:05:28Z
same rdd rule testcase
commit a740052594352f45267812fc65a04082dcf3913c
Author: jianran.tfh <ji...@taobao.com>
Date: 2017-03-16T10:07:17Z
rollback
commit b3b97086bbb5e4415e5694fcc0cd33f4511645a9
Author: jianran.tfh <ji...@taobao.com>
Date: 2017-03-16T10:17:30Z
remove get
----
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for partition...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17313
I don't understand why you say this test doesn't make sense. The assertion is correct. You say you think the second assertion should always pass, but, isn't that the purpose of the test?
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for partition...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17313
I'm also not sure about the intent of these lines and it is possible the comment doesn't quite reflect the call that follows because it refers to get(). However it seems like the tests are still valid. Unless you know they are wrong yet pass then i would leave it.
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for partition...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17313
Unless we are sure, I don't think it is worth changing.
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for pa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17313
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for partition...
Posted by jianran <gi...@git.apache.org>.
Github user jianran commented on the issue:
https://github.com/apache/spark/pull/17313
I am sorry,you are right, the second assertion is ok; but what do you think about the assertion comment:_Do a get() on rdd_0_2 so that it is the most recently used item_, what is the purpose of the test? lru or same rdd rule ; the reason for rdd_0_2 should *not* be dropped is the same rdd rule, not the comment said _Do a get() on rdd_0_2 so that it is the most recently used item_, so i think the comment is wrong or the assertion is wrong; I am sorry, if the purpose of the test is only to test rdd_0_2 was not in store, it is ok, I didn't understand the purpose or the comment of the assertion, I am sorry;
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for partition...
Posted by jianran <gi...@git.apache.org>.
Github user jianran commented on the issue:
https://github.com/apache/spark/pull/17313
do you agree remove the comment is more appreciable? can i only remove the comment?
---
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 #17313: [SPARK-19974][Block Manager] in-memory LRU for partition...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17313
Can one of the admins verify 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