Skip to content

[ZEPPELIN-4821] Spark interpreter uses incorrect property name#3773

Closed
alexott wants to merge 1 commit intoapache:masterfrom
alexott:ZEPPELIN-4821
Closed

[ZEPPELIN-4821] Spark interpreter uses incorrect property name#3773
alexott wants to merge 1 commit intoapache:masterfrom
alexott:ZEPPELIN-4821

Conversation

@alexott
Copy link
Copy Markdown
Contributor

@alexott alexott commented May 17, 2020 •
edited
Loading

What is this PR for?

Spark interpreter did use incorrect property name for Spark Master - master, although a lot of code was dependent on spark.master - it's better to use only one name everywhere

What type of PR is it?

Bug Fix

What is the Jira issue?

How should this be tested?

@alexott
Copy link
Copy Markdown
Contributor Author

alexott commented May 17, 2020

@zjffdu can you look when you have a time?

zjffdu
zjffdu reviewed May 18, 2020
View reviewed changes
Comment thread spark/interpreter/src/main/java/org/apache/zeppelin/spark/SparkInterpreter.java
}
}
// use local mode for embedded spark mode when spark.master is not found
conf.setIfMissing("spark.master", "local");
Copy link
Copy Markdown
Contributor

@zjffdu zjffdu May 18, 2020

Choose a reason for hiding this comment

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

I think this is the line of code that cause the issue in the mail-list

Copy link
Copy Markdown
Contributor Author

@alexott alexott May 18, 2020

Choose a reason for hiding this comment

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

yep, looks like...

@zjffdu
Copy link
Copy Markdown
Contributor

zjffdu commented May 18, 2020

@alexott Overall I agree with using spark.master instead of master. But I think we should keep compatibility because lots of users are still using master today.
And we should update this function to take spark.master precedence over master (https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/SparkInterpreterLauncher.java#L326) What do you think ?

@alexott
Copy link
Copy Markdown
Contributor Author

alexott commented May 18, 2020

I agree with that Jeff - I was just thinking about migration path, etc. Give me a couple of days to fix that...

@alexott alexott force-pushed the ZEPPELIN-4821 branch 3 times, most recently from f8ebf28 to e2994d8 Compare May 22, 2020 18:24
@alexott alexott force-pushed the ZEPPELIN-4821 branch 2 times, most recently from b8ef0f9 to 5433ac8 Compare June 7, 2020 11:10
@alexott alexott force-pushed the ZEPPELIN-4821 branch 2 times, most recently from 40aa11d to 9bc63f9 Compare June 7, 2020 15:41
@zjffdu
Copy link
Copy Markdown
Contributor

zjffdu commented Jun 9, 2020

@alexott is this ready to be reviewed again ?

@alexott
Copy link
Copy Markdown
Contributor Author

alexott commented Jun 10, 2020

@zjffdu not yet - I need to rebase it on top of the Travis-CI cleanup effort, and re-check again...

@alexott alexott changed the title [ZEPPELIN-4821] Spark interpreter uses incorrect property name don't merge yet [ZEPPELIN-4821] Spark interpreter uses incorrect property name Jun 10, 2020
@alexott alexott force-pushed the ZEPPELIN-4821 branch 4 times, most recently from 7f06132 to 6a8076c Compare June 14, 2020 10:22
@alexott alexott changed the title don't merge yet [ZEPPELIN-4821] Spark interpreter uses incorrect property name [ZEPPELIN-4821] Spark interpreter uses incorrect property name Jun 14, 2020
@alexott
Copy link
Copy Markdown
Contributor Author

alexott commented Jun 14, 2020

@zjffdu now it's ready for review as well... tests are finally passing.

I modified Spark launcher to use --conf spark.master instead of --master...

zjffdu
zjffdu reviewed Jun 18, 2020
View reviewed changes
Comment thread conf/zeppelin-env.cmd.template Outdated

REM set JAVA_HOME=
REM set MASTER= REM Spark master url. eg. spark://master_addr:7077. Leave empty if you want to use local mode.
REM set SPARK_MASTER= REM Spark master url. eg. spark://master_addr:7077. Leave empty if you want to use local mode.
Copy link
Copy Markdown
Contributor

@zjffdu zjffdu Jun 18, 2020

Choose a reason for hiding this comment

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

Not aligned with other rows

Copy link
Copy Markdown
Contributor Author

@alexott alexott Jun 18, 2020

Choose a reason for hiding this comment

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

yeah, the both files contained a mix of tabs & spaces, why it wasn't catch up...
Fixed in both

zjffdu
zjffdu reviewed Jun 18, 2020
View reviewed changes
Comment thread conf/zeppelin-env.sh.template Outdated

# export JAVA_HOME=
# export MASTER= # Spark master url. eg. spark://master_addr:7077. Leave empty if you want to use local mode.
# export SPARK_MASTER= # Spark master url. eg. spark://master_addr:7077. Leave empty if you want to use local mode.
Copy link
Copy Markdown
Contributor

@zjffdu zjffdu Jun 18, 2020

Choose a reason for hiding this comment

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

Not aligned with other rows

@zjffdu
Copy link
Copy Markdown
Contributor

zjffdu commented Jun 18, 2020

LGTM

@asfgit asfgit closed this in a282e78 Jun 18, 2020
asfgit pushed a commit that referenced this pull request Jun 18, 2020
@alexott
[ZEPPELIN-4821] Spark interpreter uses incorrect property name
ff03bda 
### What is this PR for?

Spark interpreter did use incorrect property name for Spark Master - `master`, although a lot of code was dependent on `spark.master` - it's better to use only one name everywhere

### What type of PR is it?
Bug Fix

### What is the Jira issue?
* ZEPPELIN-4821

### How should this be tested?
* https://travis-ci.org/github/alexott/zeppelin/builds/699582429

Author: Alex Ott <alexott@apache.org>

Closes #3773 from alexott/ZEPPELIN-4821 and squashes the following commits:

82b8321 [Alex Ott] [ZEPPELIN-4821] Spark interpreter uses incorrect property name

(cherry picked from commit a282e78)
Signed-off-by: Alex Ott <alexott@apache.org>
@alexott alexott deleted the ZEPPELIN-4821 branch June 18, 2020 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zjffdu zjffdu zjffdu left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexott @zjffdu