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.

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

[SPARK-15541] Casting ConcurrentHashMap to ConcurrentMap (branch-1.6) #14390

Closed
wants to merge 5 commits into from
Closed

[SPARK-15541] Casting ConcurrentHashMap to ConcurrentMap (branch-1.6) #14390

wants to merge 5 commits into from

Conversation

maver1ck
Copy link
Contributor

What changes were proposed in this pull request?

Casting ConcurrentHashMap to ConcurrentMap allows to run code compiled with Java 8 on Java 7

How was this patch tested?

Compilation. Existing automatic tests

@SparkQA
Copy link

SparkQA commented Jul 28, 2016

Test build #62959 has finished for PR 14390 at commit 921e3c8.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 28, 2016

Test build #62960 has finished for PR 14390 at commit 3ffbff1.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 28, 2016

Test build #62963 has finished for PR 14390 at commit 7bd4f44.

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

@maver1ck
Copy link
Contributor Author

@jkbradley
Could you look at it ?
I think this is problem from: https://issues.apache.org/jira/browse/SPARK-10086
Maybe we should merge this PR to branch-1.6 before testing ?
https://github.com/apache/spark/pull/10909/files

@srowen
Copy link
Member

srowen commented Jul 28, 2016

LGTM

@srowen
Copy link
Member

srowen commented Jul 28, 2016

... but this needs to be opened vs master first. Don't worry about the flaky test here.

@srowen
Copy link
Member

srowen commented Jul 31, 2016

Ping @maver1ck

@maver1ck
Copy link
Contributor Author

maver1ck commented Aug 2, 2016

I added another PR #14459 vs master. Using following command to find suspicious code.

for i in `grep -c -R ConcurrentHashMap | grep -v ':0' | sed -e s/:.*//`; do echo $i; grep keySet $i ; done

@maver1ck maver1ck changed the title [SPARK-15541] Casting ConcurrentHashMap to ConcurrentMap [SPARK-15541] Casting ConcurrentHashMap to ConcurrentMap (branch-1.6) Aug 2, 2016
@srowen
Copy link
Member

srowen commented Aug 2, 2016

Can you close this PR please?

@maver1ck
Copy link
Contributor Author

maver1ck commented Aug 2, 2016

Could you tell me why ?
We need different PR against different branches.
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Catalog.scala should be modified only in branch-1.6

@srowen
Copy link
Member

srowen commented Aug 2, 2016

We merge to master first and then cherry pick into other branches. Unless the patch is quite different in other branches, we can just merge it into others automatically.

@maver1ck
Copy link
Contributor Author

maver1ck commented Aug 2, 2016

I know that.
And this patch is quite different. On master there are changes only in Dispatcher.scala.
On branch-1.6 we need changes also in Catalog.scala.

So maybe this patch should add only changes in Catalog.scala ? (and we we'll patch it after #14459)

@maver1ck
Copy link
Contributor Author

maver1ck commented Aug 2, 2016

As you merged #14459 I removed changes in Dispatcher.scala.

@SparkQA
Copy link

SparkQA commented Aug 2, 2016

Test build #63131 has finished for PR 14390 at commit ea2810f.

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

@srowen
Copy link
Member

srowen commented Aug 2, 2016

Ah, I get it now. I will merge this too then. Thank you.

asfgit pushed a commit that referenced this pull request Aug 2, 2016
## What changes were proposed in this pull request?

Casting ConcurrentHashMap to ConcurrentMap allows to run code compiled with Java 8 on Java 7

## How was this patch tested?

Compilation. Existing automatic tests

Author: Maciej Brynski <[email protected]>

Closes #14390 from maver1ck/spark-15541.
@srowen
Copy link
Member

srowen commented Aug 2, 2016

Merged. @maver1ck you'll have to manually close it since the ASF bot won't close PRs not open vs master.

zzcclp pushed a commit to zzcclp/spark that referenced this pull request Aug 3, 2016
## What changes were proposed in this pull request?

Casting ConcurrentHashMap to ConcurrentMap allows to run code compiled with Java 8 on Java 7

## How was this patch tested?

Compilation. Existing automatic tests

Author: Maciej Brynski <[email protected]>

Closes apache#14390 from maver1ck/spark-15541.

(cherry picked from commit 797e758)
@maver1ck
Copy link
Contributor Author

maver1ck commented Aug 3, 2016

Done.
Thank you.

@maver1ck maver1ck closed this Aug 3, 2016
@maver1ck
Copy link
Contributor Author

maver1ck commented Aug 4, 2016

@srowen
I missed one change in Catalog.scala

@maver1ck maver1ck reopened this Aug 4, 2016
@zzcclp
Copy link
Contributor

zzcclp commented Aug 4, 2016

@maver1ck I think you need to open another pr for change in Catalog.scala.

@SparkQA
Copy link

SparkQA commented Aug 4, 2016

Test build #63220 has finished for PR 14390 at commit f009ee7.

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

@srowen
Copy link
Member

srowen commented Aug 4, 2016

@maver1ck this is already merged, leave it closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants