You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@twill.apache.org by gsps1 <gi...@git.apache.org> on 2015/10/08 00:41:16 UTC
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
GitHub user gsps1 opened a pull request:
https://github.com/apache/incubator-twill/pull/66
TWILL-63, combining twill classes into a twill.jar and having other u…
…ser classes into a program.jar, also caching the twill.jar in YarnTwillRunnerService and the content is reused across TwillApplications started by the runner
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gsps1/incubator-twill feature/twill-63-separating-twill-jars
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-twill/pull/66.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #66
----
commit 5d9bc9ff2df636f6881e4a091d0798cbca09254d
Author: shankar <sh...@cask.co>
Date: 2015-10-07T21:59:35Z
TWILL-63, combining twill classes into a twill.jar and having other user classes into a program.jar, also caching the twill.jar in YarnTwillRunnerService and the content is reused across TwillApplications started by the runner
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44343712
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -145,41 +145,63 @@ public void createBundle(Location target, Class<?> clz, Class<?>...classes) thro
* @throws IOException
*/
public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<URI> resources) throws IOException {
- LOG.debug("start creating bundle {}. building a temporary file locally at first", target.getName());
+ createBundleAndGetClassPathUrls(target, classes, resources);
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(Location target, Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ return createBundleAndGetClassPathUrls(target.getName(), target.toString(),
+ target.getOutputStream(), classes, resources);
--- End diff --
Also, this leaks the output stream resource.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41659626
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -175,6 +187,36 @@ protected void shutDown() throws Exception {
};
}
+ public Location createTwillJar() throws IOException {
+ ApplicationBundler applicationBundler = new ApplicationBundler(new ClassAcceptor());
+ List<Class<?>> classes = Lists.newArrayList();
+ classes.add(ApplicationMasterMain.class);
+ classes.add(TwillContainerMain.class);
+ // Stuck in the yarnAppClient class to make bundler being able to pickup the right yarn-client version
+ classes.add(yarnAppClient.getClass());
+
+ twillDependencyClasses = getTwillDependencyClasses(classes);
+ File tempFile = File.createTempFile("twill", ".jar");
+ Location twillJar = new LocalLocationFactory().create(tempFile.toURI());
--- End diff --
Why need to wrap it to location? Can't it be just `File`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44346230
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -145,41 +145,63 @@ public void createBundle(Location target, Class<?> clz, Class<?>...classes) thro
* @throws IOException
*/
public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<URI> resources) throws IOException {
- LOG.debug("start creating bundle {}. building a temporary file locally at first", target.getName());
+ createBundleAndGetClassPathUrls(target, classes, resources);
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(Location target, Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ return createBundleAndGetClassPathUrls(target.getName(), target.toString(),
+ target.getOutputStream(), classes, resources);
+ }
+
+ private Set<URL> createBundleAndGetClassPathUrls(String targetName, String targetPath,
+ OutputStream targetOutputStream,
+ Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ LOG.debug("start creating bundle {}. building a temporary file locally at first", targetName);
// Write the jar to local tmp file first
- File tmpJar = File.createTempFile(target.getName(), ".tmp");
+ File tmpJar = File.createTempFile(targetName, ".tmp");
+ Set<URL> classPathUrls;
try {
Set<String> entries = Sets.newHashSet();
try (JarOutputStream jarOut = new JarOutputStream(new FileOutputStream(tmpJar))) {
// Find class dependencies
- findDependencies(classes, entries, jarOut);
+ classPathUrls = findDependenciesAndGetClassPathUrls(classes, entries, jarOut);
// Add extra resources
for (URI resource : resources) {
copyResource(resource, entries, jarOut);
}
}
- LOG.debug("copying temporary bundle to destination {} ({} bytes)", target, tmpJar.length());
+ LOG.debug("copying temporary bundle to destination {} ({} bytes)", targetPath, tmpJar.length());
// Copy the tmp jar into destination.
try {
- OutputStream os = new BufferedOutputStream(target.getOutputStream());
+ OutputStream os = new BufferedOutputStream(targetOutputStream);
try {
Files.copy(tmpJar, os);
} finally {
Closeables.closeQuietly(os);
}
} catch (IOException e) {
- throw new IOException("failed to copy bundle from " + tmpJar.toURI() + " to " + target, e);
+ throw new IOException("failed to copy bundle from " + tmpJar.toURI() + " to " + targetPath, e);
}
- LOG.debug("finished creating bundle at {}", target);
+ LOG.debug("finished creating bundle at {}", targetPath);
} finally {
tmpJar.delete();
LOG.debug("cleaned up local temporary for bundle {}", tmpJar.toURI());
}
+ return classPathUrls;
+
--- End diff --
remove extra new line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41459295
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -122,15 +122,16 @@ public boolean accept(String className, URL classUrl, URL classPathUrl) {
});
}
- public void createBundle(Location target, Iterable<Class<?>> classes) throws IOException {
- createBundle(target, classes, ImmutableList.<URI>of());
+ public void createBundle(Location target, Iterable<Class<?>> classes, boolean skipTwillClasses) throws IOException {
--- End diff --
Why need to introduce this specific boolean? Can't we use the constructor that takes `ClassAcceptor`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41459679
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -137,7 +139,7 @@
YarnTwillPreparer(YarnConfiguration yarnConfig, TwillSpecification twillSpec,
YarnAppClient yarnAppClient, ZKClient zkClient,
LocationFactory locationFactory, String extraOptions, LogEntry.Level logLevel,
- YarnTwillControllerFactory controllerFactory) {
+ YarnTwillControllerFactory controllerFactory, byte[] twillJarContent) {
--- End diff --
Shouldn't be `byte[]`. Better be a `InputSupplier<? extends InputStream>`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41661046
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -175,6 +187,36 @@ protected void shutDown() throws Exception {
};
}
+ public Location createTwillJar() throws IOException {
+ ApplicationBundler applicationBundler = new ApplicationBundler(new ClassAcceptor());
+ List<Class<?>> classes = Lists.newArrayList();
+ classes.add(ApplicationMasterMain.class);
+ classes.add(TwillContainerMain.class);
+ // Stuck in the yarnAppClient class to make bundler being able to pickup the right yarn-client version
+ classes.add(yarnAppClient.getClass());
+
+ twillDependencyClasses = getTwillDependencyClasses(classes);
+ File tempFile = File.createTempFile("twill", ".jar");
+ Location twillJar = new LocalLocationFactory().create(tempFile.toURI());
+ applicationBundler.createBundle(twillJar, classes);
+ return twillJar;
+ }
+
+ private Set<String> getTwillDependencyClasses(List<Class<?>> classes) throws IOException {
+ Iterable<String> classNames = Iterables.transform(classes, new Function<Class<?>, String>() {
+ @Override
+ public String apply(Class<?> input) {
+ return input.getName();
+ }
+ });
+
+ ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
+ if (classLoader == null) {
+ classLoader = getClass().getClassLoader();
+ }
+ return Dependencies.getClassDependencies(classLoader, new ClassAcceptor(), classNames);
--- End diff --
The `getClassDependencies` doesn't give you all twill classes. It only gives you all classes in the dependencies. I think the better way is using all classPathURL encountered during the creation of the twill jar to create a `URLClassLoader`. To check if a class is inside the twill.jar, you can use `ClassLoader.getResource()` on the `URLClassLoader`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44343302
--- Diff: twill-common/src/main/java/org/apache/twill/internal/Constants.java ---
@@ -64,8 +68,8 @@
public static final class Files {
public static final String LAUNCHER_JAR = "launcher.jar";
- public static final String APP_MASTER_JAR = "appMaster.jar";
- public static final String CONTAINER_JAR = "container.jar";
+ public static final String TWILL_JAR = "twill.jar";
+ public static final String PROGRAM_JAR = "program.jar";
--- End diff --
Better call it `application.jar`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41659508
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -175,6 +187,36 @@ protected void shutDown() throws Exception {
};
}
+ public Location createTwillJar() throws IOException {
--- End diff --
Why public?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by gsps1 <gi...@git.apache.org>.
Github user gsps1 commented on the pull request:
https://github.com/apache/incubator-twill/pull/66#issuecomment-151940449
@chtyim have addressed your comments, please review, thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41659573
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -175,6 +187,36 @@ protected void shutDown() throws Exception {
};
}
+ public Location createTwillJar() throws IOException {
+ ApplicationBundler applicationBundler = new ApplicationBundler(new ClassAcceptor());
+ List<Class<?>> classes = Lists.newArrayList();
+ classes.add(ApplicationMasterMain.class);
+ classes.add(TwillContainerMain.class);
+ // Stuck in the yarnAppClient class to make bundler being able to pickup the right yarn-client version
+ classes.add(yarnAppClient.getClass());
+
+ twillDependencyClasses = getTwillDependencyClasses(classes);
+ File tempFile = File.createTempFile("twill", ".jar");
--- End diff --
Potentially this file can be deleted by the system when the runner process runs for a long time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by gsps1 <gi...@git.apache.org>.
Github user gsps1 commented on the pull request:
https://github.com/apache/incubator-twill/pull/66#issuecomment-146699555
Thanks for the review @chtyim , i have addressed your comments, please review again, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44343615
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -145,41 +145,63 @@ public void createBundle(Location target, Class<?> clz, Class<?>...classes) thro
* @throws IOException
*/
public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<URI> resources) throws IOException {
- LOG.debug("start creating bundle {}. building a temporary file locally at first", target.getName());
+ createBundleAndGetClassPathUrls(target, classes, resources);
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(Location target, Iterable<Class<?>> classes,
--- End diff --
Who call this? Seems like it's only called in this class (from above method). Maybe the private one can be called directly?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41459500
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -178,8 +181,42 @@ public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<U
}
}
+ /**
+ * Creates a {@link ByteArrayOutputStream} which includes all the given classes and
+ * all the classes that they depended on.
+ * The {@link ByteArrayOutputStream}
+ * will also include all classes and resources under the packages as given as include packages
+ * in the constructor.
+ *
+ * @param resources Extra resources to put into the jar file. If resource is a jar file, it'll be put under
+ * lib/ entry, otherwise under the resources/ entry.
+ * @param classes Set of classes to start the dependency traversal.
+ * @return ByteArrayOutputStream
+ * @throws IOException
+ */
+ public ByteArrayOutputStream getBundleAsStream(Iterable<Class<?>> classes,
--- End diff --
Also, why this method is in ApplicationBundler? Seems totally unrelated to what the role of the class is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41459425
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -178,8 +181,42 @@ public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<U
}
}
+ /**
+ * Creates a {@link ByteArrayOutputStream} which includes all the given classes and
+ * all the classes that they depended on.
+ * The {@link ByteArrayOutputStream}
+ * will also include all classes and resources under the packages as given as include packages
+ * in the constructor.
+ *
+ * @param resources Extra resources to put into the jar file. If resource is a jar file, it'll be put under
+ * lib/ entry, otherwise under the resources/ entry.
+ * @param classes Set of classes to start the dependency traversal.
+ * @return ByteArrayOutputStream
+ * @throws IOException
+ */
+ public ByteArrayOutputStream getBundleAsStream(Iterable<Class<?>> classes,
--- End diff --
So this could end up using a lot of heap space (and double it when `ByteArrayOutputStream.toByteArray()` is being called). Why it needs to be kept all in memory?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44343897
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -145,41 +145,63 @@ public void createBundle(Location target, Class<?> clz, Class<?>...classes) thro
* @throws IOException
*/
public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<URI> resources) throws IOException {
- LOG.debug("start creating bundle {}. building a temporary file locally at first", target.getName());
+ createBundleAndGetClassPathUrls(target, classes, resources);
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(Location target, Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ return createBundleAndGetClassPathUrls(target.getName(), target.toString(),
+ target.getOutputStream(), classes, resources);
+ }
+
+ private Set<URL> createBundleAndGetClassPathUrls(String targetName, String targetPath,
+ OutputStream targetOutputStream,
--- End diff --
If it takes output stream, then why need to take name and path? You either take a `Location` or `File`, then create an output stream and manage and closing it in this method, or you take a OutputStream and write to it, without caring where does it actually output to.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44348039
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -175,6 +189,27 @@ protected void shutDown() throws Exception {
};
}
+ private File createTwillJar() throws IOException {
+ ApplicationBundler applicationBundler = new ApplicationBundler(new ClassAcceptor());
+ List<Class<?>> classes = Lists.newArrayList();
+ classes.add(ApplicationMasterMain.class);
+ classes.add(TwillContainerMain.class);
+ // Stuck in the yarnAppClient class to make bundler being able to pickup the right yarn-client version
+ classes.add(yarnAppClient.getClass());
+ String parentDir = yarnConfig.get(Constants.TWILL_JAR_LOCATION, Constants.DEFAULT_TWILL_JAR_LOCATION);
+ File twillJarFile = new File(parentDir + Constants.Files.TWILL_JAR);
+
+ Set<URL> twillClassPathURLs = applicationBundler.createBundleAndGetClassPathUrls(twillJarFile,
+ classes, ImmutableList.<URI>of());
+ List<URL> normalizeTwillURLs = new ArrayList<>();
+ for (URL url : twillClassPathURLs) {
+ normalizeTwillURLs.add(new File(url.getPath()).toURI().toURL());
+ }
+
+ twillClassLoader = new URLClassLoader(normalizeTwillURLs.toArray(new URL[normalizeTwillURLs.size()]), null);
--- End diff --
Oh, seems like it will get reused. That's ok then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by gsps1 <gi...@git.apache.org>.
Github user gsps1 commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r41460247
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -122,15 +122,16 @@ public boolean accept(String className, URL classUrl, URL classPathUrl) {
});
}
- public void createBundle(Location target, Iterable<Class<?>> classes) throws IOException {
- createBundle(target, classes, ImmutableList.<URI>of());
+ public void createBundle(Location target, Iterable<Class<?>> classes, boolean skipTwillClasses) throws IOException {
--- End diff --
yeah, i should use that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44347994
--- Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -175,6 +189,27 @@ protected void shutDown() throws Exception {
};
}
+ private File createTwillJar() throws IOException {
+ ApplicationBundler applicationBundler = new ApplicationBundler(new ClassAcceptor());
+ List<Class<?>> classes = Lists.newArrayList();
+ classes.add(ApplicationMasterMain.class);
+ classes.add(TwillContainerMain.class);
+ // Stuck in the yarnAppClient class to make bundler being able to pickup the right yarn-client version
+ classes.add(yarnAppClient.getClass());
+ String parentDir = yarnConfig.get(Constants.TWILL_JAR_LOCATION, Constants.DEFAULT_TWILL_JAR_LOCATION);
+ File twillJarFile = new File(parentDir + Constants.Files.TWILL_JAR);
+
+ Set<URL> twillClassPathURLs = applicationBundler.createBundleAndGetClassPathUrls(twillJarFile,
+ classes, ImmutableList.<URI>of());
+ List<URL> normalizeTwillURLs = new ArrayList<>();
+ for (URL url : twillClassPathURLs) {
+ normalizeTwillURLs.add(new File(url.getPath()).toURI().toURL());
+ }
+
+ twillClassLoader = new URLClassLoader(normalizeTwillURLs.toArray(new URL[normalizeTwillURLs.size()]), null);
--- End diff --
Who will close this ClassLoader?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44348453
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -145,41 +145,63 @@ public void createBundle(Location target, Class<?> clz, Class<?>...classes) thro
* @throws IOException
*/
public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<URI> resources) throws IOException {
- LOG.debug("start creating bundle {}. building a temporary file locally at first", target.getName());
+ createBundleAndGetClassPathUrls(target, classes, resources);
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(Location target, Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ return createBundleAndGetClassPathUrls(target.getName(), target.toString(),
+ target.getOutputStream(), classes, resources);
+ }
+
+ private Set<URL> createBundleAndGetClassPathUrls(String targetName, String targetPath,
+ OutputStream targetOutputStream,
+ Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ LOG.debug("start creating bundle {}. building a temporary file locally at first", targetName);
// Write the jar to local tmp file first
- File tmpJar = File.createTempFile(target.getName(), ".tmp");
+ File tmpJar = File.createTempFile(targetName, ".tmp");
+ Set<URL> classPathUrls;
try {
Set<String> entries = Sets.newHashSet();
try (JarOutputStream jarOut = new JarOutputStream(new FileOutputStream(tmpJar))) {
// Find class dependencies
- findDependencies(classes, entries, jarOut);
+ classPathUrls = findDependenciesAndGetClassPathUrls(classes, entries, jarOut);
// Add extra resources
for (URI resource : resources) {
copyResource(resource, entries, jarOut);
}
}
- LOG.debug("copying temporary bundle to destination {} ({} bytes)", target, tmpJar.length());
+ LOG.debug("copying temporary bundle to destination {} ({} bytes)", targetPath, tmpJar.length());
// Copy the tmp jar into destination.
try {
- OutputStream os = new BufferedOutputStream(target.getOutputStream());
+ OutputStream os = new BufferedOutputStream(targetOutputStream);
try {
Files.copy(tmpJar, os);
} finally {
Closeables.closeQuietly(os);
}
} catch (IOException e) {
- throw new IOException("failed to copy bundle from " + tmpJar.toURI() + " to " + target, e);
+ throw new IOException("failed to copy bundle from " + tmpJar.toURI() + " to " + targetPath, e);
}
- LOG.debug("finished creating bundle at {}", target);
+ LOG.debug("finished creating bundle at {}", targetPath);
} finally {
tmpJar.delete();
LOG.debug("cleaned up local temporary for bundle {}", tmpJar.toURI());
}
+ return classPathUrls;
+
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(File target, Iterable<Class<?>> classes,
--- End diff --
no doc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44342963
--- Diff: twill-common/src/main/java/org/apache/twill/internal/Constants.java ---
@@ -51,6 +51,10 @@
public static final String RESTART_ALL_RUNNABLE_INSTANCES = "restartAllRunnableInstances";
public static final String RESTART_RUNNABLES_INSTANCES = "restartRunnablesInstances";
+ /** Twill Jar location configuration name and the default value for that config */
+ public static final String TWILL_JAR_LOCATION = "twill.jar.location";
+ public static final String DEFAULT_TWILL_JAR_LOCATION = "/tmp/";
--- End diff --
Default should be the system tmp, which may not be `/tmp`. You can get it through `System.getProperty("java.io.tmpdir")`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44344057
--- Diff: twill-common/src/main/java/org/apache/twill/internal/Constants.java ---
@@ -51,6 +51,10 @@
public static final String RESTART_ALL_RUNNABLE_INSTANCES = "restartAllRunnableInstances";
public static final String RESTART_RUNNABLES_INSTANCES = "restartRunnablesInstances";
+ /** Twill Jar location configuration name and the default value for that config */
+ public static final String TWILL_JAR_LOCATION = "twill.jar.location";
--- End diff --
What is the location? Is it a local directory or a directory based on the `LocationFactory`? Also, better make it clear that it's a directory.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by gsps1 <gi...@git.apache.org>.
Github user gsps1 commented on the pull request:
https://github.com/apache/incubator-twill/pull/66#issuecomment-160762781
thanks for the review @chtyim , @hsaputra i will be addressing the comments soon, got busy with other things. will update soon. Thanks for following up!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/66#discussion_r44346146
--- Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -145,41 +145,63 @@ public void createBundle(Location target, Class<?> clz, Class<?>...classes) thro
* @throws IOException
*/
public void createBundle(Location target, Iterable<Class<?>> classes, Iterable<URI> resources) throws IOException {
- LOG.debug("start creating bundle {}. building a temporary file locally at first", target.getName());
+ createBundleAndGetClassPathUrls(target, classes, resources);
+ }
+
+ public Set<URL> createBundleAndGetClassPathUrls(Location target, Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ return createBundleAndGetClassPathUrls(target.getName(), target.toString(),
+ target.getOutputStream(), classes, resources);
+ }
+
+ private Set<URL> createBundleAndGetClassPathUrls(String targetName, String targetPath,
+ OutputStream targetOutputStream,
+ Iterable<Class<?>> classes,
+ Iterable<URI> resources) throws IOException {
+ LOG.debug("start creating bundle {}. building a temporary file locally at first", targetName);
// Write the jar to local tmp file first
- File tmpJar = File.createTempFile(target.getName(), ".tmp");
+ File tmpJar = File.createTempFile(targetName, ".tmp");
--- End diff --
So this will be using the system temp directory. Maybe better use the one that user configurated
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill pull request: TWILL-63, combining twill classes in...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/incubator-twill/pull/66#issuecomment-160559379
Any update on this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-twill issue #66: TWILL-63, combining twill classes into a twill.ja...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the issue:
https://github.com/apache/incubator-twill/pull/66
Could you kindly move the PR to the new Twill repo: https://github.com/apache/twill
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---