Skip to content

Remove duplicated java option concats in common.sh - #749

Closed
zhongneu wants to merge 2 commits into
apache:masterfrom
zhongneu:fix-duplicated-java-opts
Closed

Remove duplicated java option concats in common.sh#749
zhongneu wants to merge 2 commits into
apache:masterfrom
zhongneu:fix-duplicated-java-opts

Conversation

@zhongneu

Copy link
Copy Markdown
Contributor

What is this PR for?

There are some java option concats in common.sh, which are executed twice when start an interpreter. This makes some of the options invalid, such as remote debugging options

What type of PR is it?

Bug Fix

Todos

Is there a relevant Jira issue?

ZEPPELIN-686

How should this be tested?

Set remote debug options:

export ZEPPELIN_INTP_JAVA_OPTS=-agentlib:jdwp=transport=dt_socket,server=y,address=8000,suspend=n

Start the Zeppelin daemon, then create & run a job to trigger starting an interpreter. The job should fail without the fix.

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update?
    NO
  • Is there breaking changes for older versions?
    NO
  • Does this needs documentation?
    NO

@Leemoonsoo

Copy link
Copy Markdown
Member

Thanks @zhongneu for the patch.
Looks good to me.

@Leemoonsoo

Copy link
Copy Markdown
Member

Merge if there're no more discsussions

@felixcheung

Copy link
Copy Markdown
Member

what if someone has set JAVA_OPTS in the env? shouldn't we be additive?

@zhongneu

Copy link
Copy Markdown
ContributorAuthor

@felixcheung I am not sure about this, but I would like to make it additive if we have such case.

@Leemoonsoo

Copy link
Copy Markdown
Member

I think @felixcheung got valid point. It's incompatible change to people who has JAVA_OPTS.
One alternative is document changes in
https://github.com/apache/incubator-zeppelin/blob/master/docs/install/upgrade.md

@felixcheung

Copy link
Copy Markdown
Member

That sounds like a reasonable approach, we could say something like
"if you are setting JAVA_OPT, set ZEPPELIN_JAVA_OPTS instead for Zeppelin, set ZEPPELIN_INTP_JAVA_OPTS instead for interpreter process" in more words ;)

@zhongneu

Copy link
Copy Markdown
ContributorAuthor

I've reverted the change for JAVA_OPTS, which is fine, because it is not used by bin/interpreter.sh

@Leemoonsoo

Copy link
Copy Markdown
Member

LGTM

@felixcheung

Copy link
Copy Markdown
Member

looks good. I think we might want to deprecate JAVA_INTP_OPTS and replace with something more Zeppelin specific for clarify, and allow for admin to override it.
merging if no more comment.

fireboy1919 pushed a commit to fireboy1919/incubator-zeppelin that referenced this pull request Apr 5, 2016
### What is this PR for?
There are some java option concats in common.sh, which are executed twice when start an interpreter. This makes some of the options invalid, such as remote debugging options
### What type of PR is it?
Bug Fix
### Todos
### Is there a relevant Jira issue?
[ZEPPELIN-686](https://issues.apache.org/jira/browse/ZEPPELIN-686)
### How should this be tested?
Set remote debug options:
```
export ZEPPELIN_INTP_JAVA_OPTS=-agentlib:jdwp=transport=dt_socket,server=y,address=8000,suspend=n
```
Start the Zeppelin daemon, then create & run a job to trigger starting an interpreter. The job should fail without the fix.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update?
NO
* Is there breaking changes for older versions?
NO
* Does this needs documentation?
NO
Author: Zhong Wang <wangzhong.neu@gmail.com>
Closesapache#749 from zhongneu/fix-duplicated-java-opts and squashes the following commits:
f89dbb6 [Zhong Wang] revert change for JAVA_OPTS for compatibility
75959ea [Zhong Wang] remove unneccessary concats in common.sh
onkarshedge pushed a commit to onkarshedge/incubator-zeppelin that referenced this pull request May 11, 2016
### What is this PR for?
There are some java option concats in common.sh, which are executed twice when start an interpreter. This makes some of the options invalid, such as remote debugging options
### What type of PR is it?
Bug Fix
### Todos
### Is there a relevant Jira issue?
[ZEPPELIN-686](https://issues.apache.org/jira/browse/ZEPPELIN-686)
### How should this be tested?
Set remote debug options:
```
export ZEPPELIN_INTP_JAVA_OPTS=-agentlib:jdwp=transport=dt_socket,server=y,address=8000,suspend=n
```
Start the Zeppelin daemon, then create & run a job to trigger starting an interpreter. The job should fail without the fix.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update?
NO
* Is there breaking changes for older versions?
NO
* Does this needs documentation?
NO
Author: Zhong Wang <wangzhong.neu@gmail.com>
Closesapache#749 from zhongneu/fix-duplicated-java-opts and squashes the following commits:
f89dbb6 [Zhong Wang] revert change for JAVA_OPTS for compatibility
75959ea [Zhong Wang] remove unneccessary concats in common.sh
Sign up for freeto 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.

3 participants

@zhongneu@Leemoonsoo@felixcheung