Skip to content

[Spark-24376][doc]Summary:compiling spark with scala-2.10 should use the -P parameter instead of -D - #21422

Closed
gentlewangyu wants to merge 2 commits into
apache:branch-2.2from
gentlewangyu:SPARK-24376
Closed

[Spark-24376][doc]Summary:compiling spark with scala-2.10 should use the -P parameter instead of -D#21422
gentlewangyu wants to merge 2 commits into
apache:branch-2.2from
gentlewangyu:SPARK-24376

Conversation

@gentlewangyu

@gentlewangyugentlewangyu commented May 24, 2018•
edited
Loading

Copy link
Copy Markdown

What changes were proposed in this pull request?

compiling spark with scala-2.10 should use the -P parameter instead of -D

How was this patch tested?

JIRA_ID:SPARK-24376
using build/mvn clean package install deploy -DskipTests -Pscala-2.10

Please review http://spark.apache.org/contributing.html before opening a pull request.

wangyu
git config --global user.email "wangyu@cmss.chinamobile.com"
d5167ed
git config --global user.name "gentlewangyu"
@gentlewangyu

gentlewangyu commented May 24, 2018

Copy link
Copy Markdown
Author

I have verified this patch

HyukjinKwon
HyukjinKwon reviewed May 24, 2018
View reviewed changes
Comment threaddocs/building-spark.md Outdated

./dev/change-scala-version.sh 2.10
./build/mvn -Pyarn -Dscala-2.10 -DskipTests clean package
./build/mvn -Pyarn -scala-2.10 -DskipTestsP clean package

@HyukjinKwonHyukjinKwonMay 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

DskipTestsP?

@gentlewangyugentlewangyuMay 24, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

sorry , It's -Pscala-2.10

@HyukjinKwon

HyukjinKwon commented May 24, 2018

Copy link
Copy Markdown
Member

Minor fix doesn't usually need a JIRA. Let's avoid next time.

@HyukjinKwon

HyukjinKwon commented May 25, 2018

Copy link
Copy Markdown
Member

ok to test

@SparkQA

SparkQA commented May 25, 2018

Copy link
Copy Markdown

Test build #91137 has finished for PR 21422 at commit bf6b801.

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

@gentlewangyu

gentlewangyu commented May 25, 2018

Copy link
Copy Markdown
Author

@HyukjinKwon thanks for your answer!
I have tested it by using the command (build/mvn clean package install deploy -DskipTests -Pscala-2.10)

image

@jerryshao

jerryshao commented May 25, 2018

Copy link
Copy Markdown
Contributor

Why do you submit PR against branch 2.2?

Besides scala-2.10 profile is no longer valid after 2.3.

@HyukjinKwon

HyukjinKwon commented May 25, 2018

Copy link
Copy Markdown
Member

But isn't it still a valid fix for 2.2.x?

@jerryshao

jerryshao commented May 25, 2018

Copy link
Copy Markdown
Contributor

Yes, but the PR should be against master branch.

@jerryshao

jerryshao commented May 25, 2018

Copy link
Copy Markdown
Contributor

And in master/Spark 2.3 code, since we don't support Scala 2.10, instead we support 2.12, so we should change that part of doc accordingly.

@jerryshao

jerryshao commented May 25, 2018

Copy link
Copy Markdown
Contributor

Seems this paragraph is already removed in master branch (https://issues.apache.org/jira/browse/SPARK-19810). So this is a branch 2.2- issue only.

@jerryshao

jerryshao commented May 29, 2018

Copy link
Copy Markdown
Contributor

Do we still have a 2.2 release? If not, then this fix seems obsolete.

@HyukjinKwon

HyukjinKwon commented May 29, 2018•
edited
Loading

Copy link
Copy Markdown
Member

I don't know but the fix is open anyway and I thought we better just merge it since it's a valid fix to branch-2.3 anyway and there's no downside of it for example like making the backport harder or since the bit is removed in the master, or potential concern of new bug in this branch alone since it's just doc fix basically. PR size is small. I got that we don't usually fix things in a specific branch alone but this one seems fine.

@jerryshao

jerryshao commented May 29, 2018

Copy link
Copy Markdown
Contributor

It is not valid for branch-2.3, 2.3 already removed the support of Scala 2.10.

@HyukjinKwon

HyukjinKwon commented May 29, 2018

Copy link
Copy Markdown
Member

Oops, I meant branch-2.2.

@HyukjinKwon

HyukjinKwon commented May 29, 2018

Copy link
Copy Markdown
Member

There was a similar(?) argument before - #18873 (comment), FYI.

@gentlewangyu, let's just close this then. The gain is small anyway.

@gentlewangyu

gentlewangyu commented May 30, 2018

Copy link
Copy Markdown
Author

@HyukjinKwon@jerryshao ok , thanks

@HyukjinKwon

HyukjinKwon commented May 30, 2018

Copy link
Copy Markdown
Member

Thank you @gentlewangyu.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@gentlewangyu@HyukjinKwon@SparkQA@jerryshao