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

ZEPPELIN-3040. Allow to specify portRange for interpreter process thrift service #2661

Closed
wants to merge 2 commits into from
Closed

ZEPPELIN-3040. Allow to specify portRange for interpreter process thrift service #2661

wants to merge 2 commits into from

Conversation

zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Nov 10, 2017

What is this PR for?

This PR is trying to add new configuration zeppelin.interpreter.portRange which control the portRange of interpreter process. This is required by some users for security reason.

What type of PR is it?

[Improvement | Feature ]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

Manually test. Set zeppelin.interpreter.portRange and launch python interpreter, verify it is in the proper portRange.

Screenshots (if appropriate)

Questions:

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

@zjffdu
Copy link
Contributor Author

zjffdu commented Nov 10, 2017 •
edited

@Leemoonsoo @jongyoul @prabhjyotsingh Could you help review it ? Thanks

felixcheung
felixcheung reviewed Nov 11, 2017
View reviewed changes
...interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterUtils.java Outdated
throws IOException {

TServerSocket tSocket = null;
// ':' is the default value which means no constraints on the portRange
if (portRange == null || portRange.equals(":")) {
Copy link
Member

Choose a reason for hiding this comment

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

what if portRange == ""?

Copy link
Member

Choose a reason for hiding this comment

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

Recently, I'm using StringUtils.isBlank which check null, empty and whitespaces only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

felixcheung
felixcheung reviewed Nov 12, 2017
View reviewed changes
helium-dev/src/main/java/org/apache/zeppelin/helium/ZeppelinDevServer.java Outdated
@@ -39,7 +39,7 @@
private DevInterpreter interpreter = null;
private InterpreterOutput out;
public ZeppelinDevServer(int port) throws TException, IOException {
super(null, port);
super(null, port, "");
Copy link
Member

Choose a reason for hiding this comment

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

maybe default to ":" like in other cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

...ne/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterManagedProcess.java
callbackHost = RemoteInterpreterUtils.findAvailableHostAddress();
callbackPort = RemoteInterpreterUtils.findRandomAvailablePortOnAllLocalInterfaces();
} catch (IOException e1) {
Copy link
Member

Choose a reason for hiding this comment

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

catch TTransportException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TTransportException is wrapped in IOException, so there it is enough to catch IOException

Copy link
Member

Choose a reason for hiding this comment

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

not from what I can see, which is why I asked:
https://github.com/apache/thrift/blob/master/lib/javame/src/org/apache/thrift/transport/TTransportException.java

https://github.com/apache/thrift/blob/master/lib/java/src/org/apache/thrift/transport/TTransportException.java

edit: or you mean IOException has an inner TTransportException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I mean the code in try catch block has wrapped TTransportException in IOException.

tSocket = RemoteInterpreterUtils.createTServerSocket(callbackPortRange);
callbackPort = tSocket.getServerSocket().getLocalPort();
callbackHost = RemoteInterpreterUtils.findAvailableHostAddress();

...ne/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterManagedProcess.java
@@ -86,7 +87,7 @@ public RemoteInterpreterManagedProcess(

@Override
public String getHost() {
return "localhost";
return host;
Copy link
Member

Choose a reason for hiding this comment

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

this could return null then (the initial value)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, getHost is used by the client of RemoteInterpreterManagedProcess for querying where this process is launched. And I think in semantic perspective it is right to return null when the process has not been launched.

@zjffdu
Copy link
Contributor Author

zjffdu commented Nov 13, 2017

Thanks @felixcheung , will merge if no more comments

@asfgit asfgit closed this in 382479f Nov 13, 2017
zjffdu added a commit to zjffdu/zeppelin that referenced this pull request Nov 16, 2017
@zjffdu
ZEPPELIN-3040. Allow to specify portRange for interpreter process thr…
aa9727a 
…ift service

This PR is trying to add new configuration `zeppelin.interpreter.portRange` which control the portRange of interpreter process. This is required by some users for security reason.

[Improvement | Feature ]

* [ ] - Task

* https://issues.apache.org/jira/browse/ZEPPELIN-3040

Manually test. Set zeppelin.interpreter.portRange and launch python interpreter, verify it is in the proper portRange.

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

Author: Jeff Zhang <zjffdu@apache.org>

Closes apache#2661 from zjffdu/ZEPPELIN-3040 and squashes the following commits:

a87c425 [Jeff Zhang] address comments
7e885bd [Jeff Zhang] ZEPPELIN-3040. Allow to specify portRange for interpreter process thrift service
zjffdu added a commit to zjffdu/zeppelin that referenced this pull request Nov 30, 2017
@zjffdu
ZEPPELIN-3040. Allow to specify portRange for interpreter process thr…
aeec35e 
…ift service

This PR is trying to add new configuration `zeppelin.interpreter.portRange` which control the portRange of interpreter process. This is required by some users for security reason.

[Improvement | Feature ]

* [ ] - Task

* https://issues.apache.org/jira/browse/ZEPPELIN-3040

Manually test. Set zeppelin.interpreter.portRange and launch python interpreter, verify it is in the proper portRange.

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

Author: Jeff Zhang <zjffdu@apache.org>

Closes apache#2661 from zjffdu/ZEPPELIN-3040 and squashes the following commits:

a87c425 [Jeff Zhang] address comments
7e885bd [Jeff Zhang] ZEPPELIN-3040. Allow to specify portRange for interpreter process thrift service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jongyoul jongyoul jongyoul left review comments

@felixcheung felixcheung felixcheung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@zjffdu @jongyoul @felixcheung