You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2018/01/16 22:56:26 UTC
[GitHub] spark pull request #20284: [SPARK-23103][core] Ensure correct sort order for...
GitHub user vanzin opened a pull request:
https://github.com/apache/spark/pull/20284
[SPARK-23103][core] Ensure correct sort order for negative values in LevelDB.
The code was sorting "0" as "less than" negative values, which is a little
wrong. Fix is simple, most of the changes are the added test and related
cleanup.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vanzin/spark SPARK-23103
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20284.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 #20284
----
commit 1f303e74b5d958a459c58a3df06ee081bbe62d18
Author: Marcelo Vanzin <va...@...>
Date: 2018-01-16T22:23:48Z
[SPARK-23103][core] Ensure correct sort order for negative values in LevelDB.
The code was sorting "0" as "less than" negative values, which is a little
wrong. Fix is simple, most of the changes are the added test and related
cleanup.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20284
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20284
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86295/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20284
**[Test build #86210 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86210/testReport)** for PR 20284 at commit [`1f303e7`](https://github.com/apache/spark/commit/1f303e74b5d958a459c58a3df06ee081bbe62d18).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20284
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20284
**[Test build #86295 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86295/testReport)** for PR 20284 at commit [`8f9d94f`](https://github.com/apache/spark/commit/8f9d94f04f9b86072d2af22e3a332525e53f7d8b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20284: [SPARK-23103][core] Ensure correct sort order for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20284
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20284
@vanzin The unit test fix is not related to the sorting issue right? Also it's totally fine to include it in this PR since it's minor.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20284
**[Test build #86210 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86210/testReport)** for PR 20284 at commit [`1f303e7`](https://github.com/apache/spark/commit/1f303e74b5d958a459c58a3df06ee081bbe62d18).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20284
It is actually. Before, negative values (empty metrics all have value "-1") would show up when starting iteration from 0, and now they don't.
```
store.view(classOf[TaskDataWrapper])
.parent(stageKey)
.index(TaskIndexNames.EXEC_RUN_TIME)
.first(0L)
.closeableIterator()
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20284
even though we don't *know* of this causing a bug in 2.3, I still think we should merge it in there just because there may be some case we aren't thinking of, and this is a relatively small, safe fix.
so, I'm merging to master & 2.3
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20284
Ah, thanks for the explanation!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20284
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20284: [SPARK-23103][core] Ensure correct sort order for...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/20284#discussion_r162166090
--- Diff: common/kvstore/src/test/java/org/apache/spark/util/kvstore/DBIteratorSuite.java ---
@@ -112,7 +114,8 @@ public void setup() throws Exception {
t.key = "key" + i;
t.id = "id" + i;
t.name = "name" + RND.nextInt(MAX_ENTRIES);
- t.num = RND.nextInt(MAX_ENTRIES);
+ // Force one item to have an integer value of zero to test the fix for SPARK-23103.
+ t.num = (i != 0) ? (int) RND.nextLong() : 0;
--- End diff --
why the chang from `RND.nextInt(MAX_ENTRIES)`? this seems fine, just seemed like you were more likely to stress collision on this index before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20284
**[Test build #86295 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86295/testReport)** for PR 20284 at commit [`8f9d94f`](https://github.com/apache/spark/commit/8f9d94f04f9b86072d2af22e3a332525e53f7d8b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20284: [SPARK-23103][core] Ensure correct sort order for negati...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20284
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86210/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20284: [SPARK-23103][core] Ensure correct sort order for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20284#discussion_r162168466
--- Diff: common/kvstore/src/test/java/org/apache/spark/util/kvstore/DBIteratorSuite.java ---
@@ -112,7 +114,8 @@ public void setup() throws Exception {
t.key = "key" + i;
t.id = "id" + i;
t.name = "name" + RND.nextInt(MAX_ENTRIES);
- t.num = RND.nextInt(MAX_ENTRIES);
+ // Force one item to have an integer value of zero to test the fix for SPARK-23103.
+ t.num = (i != 0) ? (int) RND.nextLong() : 0;
--- End diff --
`nextInt` doesn't return negative values. I can add a mod operator here, but the code already explicitly generates some clashes a few lines below.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org