----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19151/#review37106 -----------------------------------------------------------
twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68413> Check the preconditions before anything. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68414> Why you want to get the parent of system classloader? I thought you want to use system classloader as the parent of the URLClassLoader. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68415> If there is error, isn't that we want to propagate so that the container would exit with failure rather than success? Also you might want to move the class loading into the init() method of TwillRunnable and only the invocation to run() method so that if failure happened in loading class, the container would exit with initialization error so that AM won't keep retrying. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68417> Move this into the try block, then in the finally you don't need to check for null. This makes the code cleaner. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68419> The while loop could be simplified to ByteStreams.copy(is, os); twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68420> Just make unJar throw IOException and will make this method much less clutter. OutputStream os = new ...; try { InputStream is = jarFile.get...; try { ByteStreams.copy(is, os); } finally { is.close(); } } finally { os.close(); } twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68421> Url should be all caps URL. twill-ext/src/main/java/org/apache/twill/ext/RuntimeBundledJarApplication.java <https://reviews.apache.org/r/19151/#comment68425> Should it be called something like ConfigDrivenBundledJarApp? (Not necessarily the best name, but should be clearer than Runtime). twill-ext/src/main/java/org/apache/twill/ext/RuntimeBundledJarApplication.java <https://reviews.apache.org/r/19151/#comment68423> There is a org.apache.twill.internal.json.ResourceSpecificationCodec. Is that not good enough? - Terence Yim On March 13, 2014, 1:15 a.m., Jiahua Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19151/ > ----------------------------------------------------------- > > (Updated March 13, 2014, 1:15 a.m.) > > > Review request for Twill. > > > Repository: twill > > > Description > ------- > > Implement a TwillApplication that runs bundled jars. This is to allow user > programs to run without having to worry about dependency conflicts with > Twill. For example, Presto uses Guava 16 whereas Twill uses Guava 13, so they > can't run in the same class loader. > > See JarRunnerExample (in twill-examples) for example usage of > BundledJarApplication (in twill-ext). > > > Diffs > ----- > > pom.xml 8e93850 > twill-api/src/main/java/org/apache/twill/api/LocalFile.java bcc3e13 > > twill-api/src/main/java/org/apache/twill/internal/DefaultResourceSpecification.java > 2998165 > twill-examples/echo/pom.xml PRE-CREATION > twill-examples/echo/src/main/java/echo/EchoMain.java PRE-CREATION > twill-examples/pom.xml PRE-CREATION > twill-examples/src/main/java/org/apache/twill/yarn/BundledJarExample.java > PRE-CREATION > twill-examples/src/main/java/org/apache/twill/yarn/HelloWorld.java > PRE-CREATION > > twill-examples/src/main/java/org/apache/twill/yarn/RuntimeBundledJarExample.java > PRE-CREATION > twill-ext/pom.xml PRE-CREATION > twill-ext/src/main/java/org/apache/twill/ext/BundledJarApplication.java > PRE-CREATION > twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunnable.java > PRE-CREATION > twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java > PRE-CREATION > > twill-ext/src/main/java/org/apache/twill/ext/RuntimeBundledJarApplication.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/19151/diff/ > > > Testing > ------- > > Tested JarRunnerExample on a single node cluster. Also tested a Twill > application that starts up multiple runnables that run Presto services. > > > Thanks, > > Jiahua Wang > >
