edwardcapriolo commented on code in PR #506:
URL: https://github.com/apache/incubator-livy/pull/506#discussion_r2701557352
##########
api/src/main/java/org/apache/livy/LivyClientBuilder.java:
##########
@@ -134,20 +131,24 @@ public LivyClient build() {
LivyClient client = null;
if (CLIENT_FACTORIES.isEmpty()) {
- throw new IllegalStateException("No LivyClientFactory implementation was
found.");
+ throw new IllegalStateException("No LivyClientFactory implementations
were found.");
}
for (LivyClientFactory factory : CLIENT_FACTORIES) {
try {
- client = factory.createClient(uri, config);
- } catch (Exception e) {
- if (!(e instanceof RuntimeException)) {
- e = new RuntimeException(e);
+ Optional<LivyClient> found = factory.createClient(uri, config);
Review Comment:
This is still a slightly "funky" pattern. It might be better to make an
interface with two methods. One advertising what protocols the Client supports,
then the other returning the Client. I went with returning Optional instead of
sometimes returning null. You might want to take it or leave it. If people hate
it the other cleanups are still nice.
##########
rsc/src/main/java/org/apache/livy/rsc/ContextLauncher.java:
##########
@@ -181,13 +174,16 @@ private static ChildProcess startDriver(final RSCConf
conf, Promise<?> promise)
}
Utils.checkState(rscJars.isDirectory(),
- "Cannot find rsc jars directory under LIVY_HOME.");
+ "Cannot find rsc jars directory: " + rscJars.getAbsolutePath());
Review Comment:
Motivation: Better to tell people where it is looking exactly instead of
telling them where Livy root is.
##########
rsc/src/main/java/org/apache/livy/rsc/ContextLauncher.java:
##########
@@ -436,36 +422,34 @@ public void detach() {
try {
monitor.join(conf.getTimeAsMs(CLIENT_SHUTDOWN_TIMEOUT));
} catch (InterruptedException ie) {
- LOG.debug("Interrupted before driver thread was finished.");
+ LOG.debug("Interrupted before driver thread was finished.", ie);
}
}
private Thread monitor(final Runnable task, int childId) {
- Runnable wrappedTask = new Runnable() {
- @Override
- public void run() {
- try {
- task.run();
- } finally {
- confFile.delete();
- }
+ Runnable wrappedTask = () -> {
+ try {
+ task.run();
+ } finally {
+ boolean ignored = confFile.delete();
}
};
Thread thread = new Thread(wrappedTask);
thread.setDaemon(true);
thread.setName("ContextLauncher-" + childId);
- thread.setUncaughtExceptionHandler(new Thread.UncaughtExceptionHandler()
{
- @Override
- public void uncaughtException(Thread t, Throwable e) {
- LOG.warn("Child task threw exception.", e);
- fail(e);
- }
+ thread.setUncaughtExceptionHandler((t, e) -> {
+ LOG.warn("Child task threw exception.", e);
+ fail(e);
});
thread.start();
return thread;
}
}
+ public RSCConf getConf() {
Review Comment:
Compile claimed RSConf is unused. I didn't want to disrupt the constructor
so added a getter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
