-
Notifications
You must be signed in to change notification settings - Fork 315
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
Livy:337 Binding RPCServer to user provided port and not random port #334
Conversation
Codecov Report
@@ Coverage Diff @@
## master #334 +/- ##
============================================
+ Coverage 70.46% 70.63% +0.17%
- Complexity 726 730 +4
============================================
Files 96 96
Lines 5123 5153 +30
Branches 774 776 +2
============================================
+ Hits 3610 3640 +30
Misses 996 996
Partials 517 517
Continue to review full report at Codecov.
|
@zjffdu @alex-the-man : Build : 238d238 |
@zjffdu @alex-the-man : Could you please review the pull request. Thanks |
@zjffdu @alex-the-man : Please let me know , if there any active development going in livy . Or should I close this pull request . I am using Livy in my organization and needed these changes . |
@zjffdu : Any Update ? |
@pralabhkumar sorry for late response. It seems user need to specify 2 properties for the port which is a little complicated to me. Could we just use one property ? e.g. launcher.port.range=5000-5500 |
@zjffdu Thanks for the suggestion , working on it . Will push the new changes in couple of days |
@zjffdu . Have done the changes as suggested . launcher.port.range is the parameter which user should set . This parameter is the only parameter user should set. launcher.port is there in the RSCConf but there is no effect of user setting it . This property is now used to communicate launcher port number from ContextLauncer to RSCDriver. Please review the changes . |
@@ -61,18 +63,74 @@ | |||
private static final SecureRandom RND = new SecureRandom(); | |||
|
|||
private final String address; | |||
private final Channel channel; | |||
private Channel channel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
public RpcServer(RSCConf lconf) throws IOException, InterruptedException { | ||
this.config = lconf; | ||
this.portRange=config.get(LAUNCHER_PORT_RANGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
miss space around =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
It is weird that I found some code style issue, but CI still pass. @jerryshao Could you help check whether it is caused by the maven project structure ? |
Ok, let me check it. Maybe Java code style check is not worked well. |
}catch(BindException e){ | ||
LOG.warn("RPC not able to connect port "+ startingPortNumber); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen when there is no available port in this range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have Changed the code , to handle the case if no available port . It will throws exception and fails gracefully.
Please let me know , if that ok .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zjffdu Working on this .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zjffdu done. Graceful failure if not port available (as would have happened if the random port is not available)
I have done the changes as suggested by @zjffdu . But travis build doesn't run (don't know why) . Please review the changes . |
Facing following issue while building it in Travis . Though I have not done any changes with respect to below error ImportError: No module named six
Command "/opt/python/2.7.12/bin/python2.7 -u -c "import setuptools, tokenize;file='/tmp/pip-build-kE1PPR/configparser/setup.py';f=getattr(tokenize, 'open', open)(file);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, file, 'exec'))" install --record /tmp/pip-LNZmV5-record/install-record.txt --single-version-externally-managed --compile --user --prefix=" failed with error code 1 in /tmp/pip-build-kE1PPR/configparser/ The command "pip install --user requests pytest flaky flake8 requests-kerberos" failed and exited with 1 during . |
Please help me with above issue , i am sure it has nothing to do with my part of code . |
Looks like Travis is broken, let me try to fix it. |
@jerryshao 1 There is no port available to launch RPCServer . I used the same port which used for this build 2 Some of the builds are giving following error . Please help
|
HI jerryshao <https://github.com/jerryshao>
Thanks for fixing it above issue . But Still there are some issues ,which
may not be related my code changes . Please help
1 There is no port available to launch RPCServer . I used the same port
which used for this build
4a7e219
<4a7e219>
.
But now , its saying no port available in the range .
2 Some of the builds are giving following error . Please help
https://travis-ci.org/cloudera/livy/jobs/238365627
- basic interactive session *** FAILED *** (5 minutes, 3 seconds)
The code passed to eventually never returned normally. Attempted 305
times over 5.012684563933334 minutes. Last failure message: assertion
failed: Session 0 state dead doesn't equal one of Set(idle).
(LivyRestClient.scala:100)
- Final session state:
SessionSnapshot(0,Some(application_1496328962772_0001),dead,AppInfo(None,Some(
http://testing-gce-05fca65b-482d-4c3e-8cf6-39a9f86cb83c:36897/cluster/app/application_1496328962772_0001)),Vector(
at
org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch.call(ContainerLaunch.java:302),
at
org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch.call(ContainerLaunch.java:82),
at java.util.concurrent.FutureTask.run(FutureTask.java:262), at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145),
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615),
at java.lang.Thread.run(Thread.java:745), , , Container exited with a
non-zero exit code 15, Failing this attempt. Failing the application.))
- YARN diagnostics: Application application_1496328962772_0001 failed
1 times due to AM Container for
appattempt_1496328962772_0001_000001 exited
with exitCode: 15
For more detailed output, check application tracking page:
http://testing-gce-05fca65b-482d-4c3e-8cf6-39a9f86cb83c:36897/cluster/app/application_1496328962772_0001Then,
click on links to logs of each attempt.
Diagnostics: Exception from container-launch.
Container id: container_1496328962772_0001_01_000001
Exit code: 15
Stack trace: ExitCodeException exitCode=15:
at org.apache.hadoop.util.Shell.runCommand(Shell.java:582)
at org.apache.hadoop.util.Shell.run(Shell.java:479)
at
org.apache.hadoop.util.Shell$ShellCommandExecutor.execute(Shell.java:773)
at
org.apache.hadoop.yarn.server.nodemanager.DefaultContainerExecutor.launchContainer(DefaultContainerExecutor.java:212)
at
org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch.call(ContainerLaunch.java:302)
at
org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch.call(ContainerLaunch.java:82)
at java.util.concurrent.FutureTask.run(FutureTask.java:262)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
Container exited with a non-zero exit code 15
Failing this attempt. Failing the application.
…On Thu, Jun 1, 2017 at 6:05 PM, Saisai Shao ***@***.***> wrote:
Looks like Travis is broken, let me try to fix it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#334 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APZjN0tyN_mXfCBlP4VwOqltajzNxXDhks5r_rAZgaJpZM4NfH-m>
.
|
LOG.error("Port Range format is not correct " + this.portRange); | ||
throw e; | ||
} | ||
catch(NumberFormatException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code style issue
address = config.findLocalAddress(); | ||
} | ||
this.address = address; | ||
return channel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indention
@zjffdu Please review |
* Get Port Numbers | ||
*/ | ||
public int[] getPortNumberAndRange() throws ArrayIndexOutOfBoundsException, | ||
NumberFormatException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
@@ -111,6 +111,61 @@ public void run() { | |||
} | |||
|
|||
/** | |||
* Get Port Numbers | |||
*/ | |||
public int[] getPortNumberAndRange() throws ArrayIndexOutOfBoundsException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to private
/** | ||
* @throws InterruptedException | ||
**/ | ||
public Channel getChannel(int portNumber) throws BindException, InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
**/ | ||
public Channel getChannel(int portNumber) throws BindException, InterruptedException { | ||
Channel channel = new ServerBootstrap() | ||
.group(group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
@jerryshao Do you have any finding why the code style issue is not detected by CI ? |
No, I haven't got a chance to look at this issue. Let's check the style issue here manually. |
@zjffdu Thanks for review . Have done all the suggested changes . Please review again :) |
@zjffdu Please let me know , if we are good with the changes or further changes are needed |
@pralabhkumar Thanks for this. Almost LGTM, one minor thing left, could you add unit test in |
int endPort = Integer.parseInt(portRange[1]); | ||
for (int index = startPort; index <= endPort; index++) { | ||
RpcServer server = autoClose(new RpcServer(emptyConfig)); | ||
assertTrue(startPort <= server.getPort() && server.getPort() <= endPort); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems you are using the default value of LAUNCHER_PORT_RANGE, don't understand why using a for loop here ?
Basically I think we need to add 2 test cases. One is specifying a wrong value of LAUNCHER_PORT_RANGE and verify the correct Exception is caught, another is specifying a correct value of LAUNCHER_PORT_RANGE, and the port of server is in this range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zjffdu Thanks for your suggestion . Working on it , will push in an hour. :)
Have created the unit test case . Please review and let me know , if its ok . |
conf/livy-client.conf.template
Outdated
@@ -55,7 +55,7 @@ | |||
|
|||
# Address for the RSC driver to connect back with it's connection info. | |||
# livy.rsc.launcher.address = | |||
# livy.rsc.launcher.port = -1 | |||
# livy.rsc.launcher.port.range = 10000~10110 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add one comment for this configuration, especially about whether start/end port is inclusive and exclusive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zjffdu
done
} catch (Exception ee) { | ||
assertTrue(ee instanceof ArrayIndexOutOfBoundsException); | ||
} | ||
portRange = "11000~11001"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use 1100011110 ? Because 1100011001 is likely to fail the unit test if these 2 port are happenly being used in the travis server.
Thanks @pralabhkumar LGTM, wait for CI pass |
@zjffdu |
@zjffdu Please let me know if anything else is needed from my side. :) |
@zffdu . Thanks for merging it and really thankful for providing help through out the process. Can u please close the jira LIVY-337 https://issues.cloudera.org/browse/LIVY-337 Thanks for your support. |
#Livy:337 : Bind RPCServer to user provided port https://issues.cloudera.org/browse/LIVY-337