----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19151/#review36991 -----------------------------------------------------------
twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunnable.java <https://reviews.apache.org/r/19151/#comment68244> It would be better if the mainArgs comes from the TwillContext.getArguments() as I mentioned above. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68230> Take a File rather than String. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68242> Normally no need to final local variables unless you want to use it in inner class. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68229> delete on exit doesn't work on directory. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68233> Why need to copy before unJar? twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68243> It might be better to allow user to specify what is the directory inside the bundle jar file contains .jar and what is the directory contains .class twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68231> Why not just always require the file be exists? Isn't that when this Runnable is executed with file being localized by Twill? twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68234> Why using full qualifier vs using import? twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68236> Besides, this is not the right way of using InputSupplier, as it normally expect a new InputStream is created when getInput() is called, as the caller would close the stream. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68238> So empty directory are getting skipped? I think it's better to just unjar everything. twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68239> Use try-finally to always close stream. OutputStream os = ... try { InputStream is = ... try { // copying logic. } finally { is.close(); } } finally { os.close(); } twill-ext/src/main/java/org/apache/twill/ext/BundledJarRunner.java <https://reviews.apache.org/r/19151/#comment68240> Naming is not good. Better call it something like getJarURLs - Terence Yim On March 12, 2014, 8:14 p.m., Jiahua Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19151/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 8:14 p.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-examples/.gitignore PRE-CREATION > twill-examples/pom.xml PRE-CREATION > twill-examples/src/main/java/org/apache/twill/yarn/HelloWorld.java > PRE-CREATION > twill-examples/src/main/java/org/apache/twill/yarn/JarRunnerExample.java > PRE-CREATION > twill-examples/src/main/resources/org/apache/twill/yarn/bundled.jar.keep > 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 > > 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 > >
