Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-23103][core] Ensure correct sort order for negative values in LevelDB. #20284

Closed
wants to merge 3 commits into from
Closed

[SPARK-23103][core] Ensure correct sort order for negative values in LevelDB. #20284

wants to merge 3 commits into from

Conversation

vanzin
Copy link
Contributor

@vanzin vanzin commented Jan 16, 2018

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.

[SPARK-23103][core] Ensure correct sort order for negative values in …
1f303e7 
…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.
@SparkQA
Copy link

SparkQA commented Jan 17, 2018

Test build #86210 has finished for PR 20284 at commit 1f303e7.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin vanzin mentioned this pull request Jan 17, 2018
Closed
squito
squito approved these changes Jan 17, 2018
View reviewed changes
Copy link
Contributor

@squito squito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

jiangxb1987
jiangxb1987 approved these changes Jan 17, 2018
View reviewed changes
Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jiangxb1987
Copy link
Contributor

retest this please

@jiangxb1987
Copy link
Contributor

@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.

@vanzin
Copy link
Contributor Author

vanzin commented Jan 17, 2018

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()

@jiangxb1987
Copy link
Contributor

Ah, thanks for the explanation!

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86295 has finished for PR 20284 at commit 8f9d94f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@squito
Copy link
Contributor

squito commented Jan 19, 2018

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

asfgit pushed a commit that referenced this pull request Jan 19, 2018
@squito
[SPARK-23103][CORE] Ensure correct sort order for negative values in …
d0cb198 
…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.

Author: Marcelo Vanzin <vanzin@cloudera.com>

Closes #20284 from vanzin/SPARK-23103.

(cherry picked from commit aa3a127)
Signed-off-by: Imran Rashid <irashid@cloudera.com>
@asfgit asfgit closed this in aa3a127 Jan 19, 2018
@vanzin vanzin deleted the SPARK-23103 branch January 22, 2018 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squito squito squito approved these changes

@jiangxb1987 jiangxb1987 jiangxb1987 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vanzin @SparkQA @jiangxb1987 @squito