You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2013/07/17 03:07:11 UTC
Review Request 12618: Extended MesosNativeLibrary.java to load the native
library from an absolute path.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos
Description
-------
Used this in a subsequent review for Jenkins scheduler.
Diffs
-----
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
Diff: https://reviews.apache.org/r/12618/diff/
Testing
-------
make jenkins
Thanks,
Vinod Kone
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/#review23551
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On July 19, 2013, 5:31 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12618/
> -----------------------------------------------------------
>
> (Updated July 19, 2013, 5:31 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used this in a subsequent review for Jenkins scheduler.
>
>
> Diffs
> -----
>
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
>
> Diff: https://reviews.apache.org/r/12618/diff/
>
>
> Testing
> -------
>
> make jenkins
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/
-----------------------------------------------------------
(Updated July 19, 2013, 5:31 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's.
Repository: mesos
Description
-------
Used this in a subsequent review for Jenkins scheduler.
Diffs (updated)
-----
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
Diff: https://reviews.apache.org/r/12618/diff/
Testing
-------
make jenkins
Thanks,
Vinod Kone
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/
-----------------------------------------------------------
(Updated July 18, 2013, 9:18 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's.
Repository: mesos
Description
-------
Used this in a subsequent review for Jenkins scheduler.
Diffs (updated)
-----
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
Diff: https://reviews.apache.org/r/12618/diff/
Testing
-------
make jenkins
Thanks,
Vinod Kone
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Vinod Kone <vi...@gmail.com>.
> On July 17, 2013, 3:16 a.m., Ben Mahler wrote:
> > src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in, line 80
> > <https://reviews.apache.org/r/12618/diff/1/?file=322306#file322306line80>
> >
> > What about having the load(path) overload should only try the path provided since it's being explicitly specified?
> >
> > This way load() can call load(System.getenv("MESOS_NATIVE_LIBRARY")) when it's set, otherwise it will try the System.loadLibrary("mesos");
> >
> > i.e.
> >
> > // Tries to load at the specified path.
> > load(String path) {
> > if (loaded) return;
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.load(path);
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library at " + path);
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> > // Uses MESOS_NATIVE_LIBRARY if set in the environment. Otherwise tries
> > // to load the "mesos" library.
> > load() {
> > if (loaded) return;
> >
> > // Use MESOS_NATIVE_LIBRARY if specified.
> > if (System.getenv("MESOS_NATIVE_LIBRARY") != null) {
> > load(System.getenv("MESOS_NATIVE_LIBRARY"));
> > return;
> > }
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.loadLibrary("mesos");
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library from " +
> > System.getProperty("java.library.path"));
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> >
> > I guess the annoyance here is having to duplicate the class loader code, but the functions have a clearer behavior, right now the load(path) method does quite a lot, no?
>
> Vinod Kone wrote:
> Thanks for the code. That does look better. Fixed.
>
> Benjamin Hindman wrote:
> If we had an option type you could imagine passing in an optional path and using System.loadLibrary when the path is none. You could accomplish this with just looking to see if 'path' is null (which seems to be a standard Java thing to do without an option type). That would look like:
>
> void load(String path) {
> if (loaded) {
> return;
> }
>
> ClassLoader contextClassLoader =
> Thread.currentThread().getContextClassLoader();
> Thread.currentThread().setContextClassLoader(
> MesosNativeLibrary.class.getClassLoader());
>
> try {
> if (path != null) {
> System.load(path);
> } else {
> System.loadLibrary("mesos")
> }
> } catch (UnsatisfiedLinkError error) {
> if (path != null) {
> System.err.println("Failed to load native Mesos library at " + path);
> } else {
> System.err.println("Failed to load native Mesos library from " +
> System.getProperty("java.library.path"));
> }
> throw error;
> } finally {
> Thread.currentThread().setContextClassLoader(contextClassLoader);
> }
> }
>
> void load() {
> load(System.getenv("MESOS_NATIVE_LIBRARY"));
> }
Hmm...This looks nice too. BenM, are you ok with this?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/#review23231
-----------------------------------------------------------
On July 18, 2013, 9:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12618/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 9:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used this in a subsequent review for Jenkins scheduler.
>
>
> Diffs
> -----
>
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
>
> Diff: https://reviews.apache.org/r/12618/diff/
>
>
> Testing
> -------
>
> make jenkins
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 17, 2013, 3:16 a.m., Ben Mahler wrote:
> > src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in, line 80
> > <https://reviews.apache.org/r/12618/diff/1/?file=322306#file322306line80>
> >
> > What about having the load(path) overload should only try the path provided since it's being explicitly specified?
> >
> > This way load() can call load(System.getenv("MESOS_NATIVE_LIBRARY")) when it's set, otherwise it will try the System.loadLibrary("mesos");
> >
> > i.e.
> >
> > // Tries to load at the specified path.
> > load(String path) {
> > if (loaded) return;
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.load(path);
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library at " + path);
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> > // Uses MESOS_NATIVE_LIBRARY if set in the environment. Otherwise tries
> > // to load the "mesos" library.
> > load() {
> > if (loaded) return;
> >
> > // Use MESOS_NATIVE_LIBRARY if specified.
> > if (System.getenv("MESOS_NATIVE_LIBRARY") != null) {
> > load(System.getenv("MESOS_NATIVE_LIBRARY"));
> > return;
> > }
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.loadLibrary("mesos");
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library from " +
> > System.getProperty("java.library.path"));
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> >
> > I guess the annoyance here is having to duplicate the class loader code, but the functions have a clearer behavior, right now the load(path) method does quite a lot, no?
>
> Vinod Kone wrote:
> Thanks for the code. That does look better. Fixed.
If we had an option type you could imagine passing in an optional path and using System.loadLibrary when the path is none. You could accomplish this with just looking to see if 'path' is null (which seems to be a standard Java thing to do without an option type). That would look like:
void load(String path) {
if (loaded) {
return;
}
ClassLoader contextClassLoader =
Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(
MesosNativeLibrary.class.getClassLoader());
try {
if (path != null) {
System.load(path);
} else {
System.loadLibrary("mesos")
}
} catch (UnsatisfiedLinkError error) {
if (path != null) {
System.err.println("Failed to load native Mesos library at " + path);
} else {
System.err.println("Failed to load native Mesos library from " +
System.getProperty("java.library.path"));
}
throw error;
} finally {
Thread.currentThread().setContextClassLoader(contextClassLoader);
}
}
void load() {
load(System.getenv("MESOS_NATIVE_LIBRARY"));
}
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/#review23231
-----------------------------------------------------------
On July 18, 2013, 9:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12618/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 9:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used this in a subsequent review for Jenkins scheduler.
>
>
> Diffs
> -----
>
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
>
> Diff: https://reviews.apache.org/r/12618/diff/
>
>
> Testing
> -------
>
> make jenkins
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Ben Mahler <be...@gmail.com>.
> On July 17, 2013, 3:16 a.m., Ben Mahler wrote:
> > src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in, line 80
> > <https://reviews.apache.org/r/12618/diff/1/?file=322306#file322306line80>
> >
> > What about having the load(path) overload should only try the path provided since it's being explicitly specified?
> >
> > This way load() can call load(System.getenv("MESOS_NATIVE_LIBRARY")) when it's set, otherwise it will try the System.loadLibrary("mesos");
> >
> > i.e.
> >
> > // Tries to load at the specified path.
> > load(String path) {
> > if (loaded) return;
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.load(path);
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library at " + path);
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> > // Uses MESOS_NATIVE_LIBRARY if set in the environment. Otherwise tries
> > // to load the "mesos" library.
> > load() {
> > if (loaded) return;
> >
> > // Use MESOS_NATIVE_LIBRARY if specified.
> > if (System.getenv("MESOS_NATIVE_LIBRARY") != null) {
> > load(System.getenv("MESOS_NATIVE_LIBRARY"));
> > return;
> > }
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.loadLibrary("mesos");
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library from " +
> > System.getProperty("java.library.path"));
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> >
> > I guess the annoyance here is having to duplicate the class loader code, but the functions have a clearer behavior, right now the load(path) method does quite a lot, no?
>
> Vinod Kone wrote:
> Thanks for the code. That does look better. Fixed.
>
> Benjamin Hindman wrote:
> If we had an option type you could imagine passing in an optional path and using System.loadLibrary when the path is none. You could accomplish this with just looking to see if 'path' is null (which seems to be a standard Java thing to do without an option type). That would look like:
>
> void load(String path) {
> if (loaded) {
> return;
> }
>
> ClassLoader contextClassLoader =
> Thread.currentThread().getContextClassLoader();
> Thread.currentThread().setContextClassLoader(
> MesosNativeLibrary.class.getClassLoader());
>
> try {
> if (path != null) {
> System.load(path);
> } else {
> System.loadLibrary("mesos")
> }
> } catch (UnsatisfiedLinkError error) {
> if (path != null) {
> System.err.println("Failed to load native Mesos library at " + path);
> } else {
> System.err.println("Failed to load native Mesos library from " +
> System.getProperty("java.library.path"));
> }
> throw error;
> } finally {
> Thread.currentThread().setContextClassLoader(contextClassLoader);
> }
> }
>
> void load() {
> load(System.getenv("MESOS_NATIVE_LIBRARY"));
> }
>
> Vinod Kone wrote:
> Hmm...This looks nice too. BenM, are you ok with this?
SGTM! This was actually going to be my suggestion as well but I wasn't sure about passing null and if you guys would like that. :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/#review23231
-----------------------------------------------------------
On July 18, 2013, 9:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12618/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 9:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used this in a subsequent review for Jenkins scheduler.
>
>
> Diffs
> -----
>
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
>
> Diff: https://reviews.apache.org/r/12618/diff/
>
>
> Testing
> -------
>
> make jenkins
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Vinod Kone <vi...@gmail.com>.
> On July 17, 2013, 3:16 a.m., Ben Mahler wrote:
> > src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in, line 80
> > <https://reviews.apache.org/r/12618/diff/1/?file=322306#file322306line80>
> >
> > What about having the load(path) overload should only try the path provided since it's being explicitly specified?
> >
> > This way load() can call load(System.getenv("MESOS_NATIVE_LIBRARY")) when it's set, otherwise it will try the System.loadLibrary("mesos");
> >
> > i.e.
> >
> > // Tries to load at the specified path.
> > load(String path) {
> > if (loaded) return;
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.load(path);
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library at " + path);
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> > // Uses MESOS_NATIVE_LIBRARY if set in the environment. Otherwise tries
> > // to load the "mesos" library.
> > load() {
> > if (loaded) return;
> >
> > // Use MESOS_NATIVE_LIBRARY if specified.
> > if (System.getenv("MESOS_NATIVE_LIBRARY") != null) {
> > load(System.getenv("MESOS_NATIVE_LIBRARY"));
> > return;
> > }
> >
> > ClassLoader contextClassLoader =
> > Thread.currentThread().getContextClassLoader();
> > Thread.currentThread().setContextClassLoader(
> > MesosNativeLibrary.class.getClassLoader());
> >
> > try {
> > System.loadLibrary("mesos");
> > } catch (UnsatisfiedLinkError error) {
> > System.err.println("Failed to load native Mesos library from " +
> > System.getProperty("java.library.path"));
> > throw error;
> > } finally {
> > Thread.currentThread().setContextClassLoader(contextClassLoader);
> > }
> > }
> >
> >
> > I guess the annoyance here is having to duplicate the class loader code, but the functions have a clearer behavior, right now the load(path) method does quite a lot, no?
Thanks for the code. That does look better. Fixed.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/#review23231
-----------------------------------------------------------
On July 17, 2013, 1:07 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12618/
> -----------------------------------------------------------
>
> (Updated July 17, 2013, 1:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used this in a subsequent review for Jenkins scheduler.
>
>
> Diffs
> -----
>
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
>
> Diff: https://reviews.apache.org/r/12618/diff/
>
>
> Testing
> -------
>
> make jenkins
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 12618: Extended MesosNativeLibrary.java to load the
native library from an absolute path.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12618/#review23231
-----------------------------------------------------------
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
<https://reviews.apache.org/r/12618/#comment47121>
What about having the load(path) overload should only try the path provided since it's being explicitly specified?
This way load() can call load(System.getenv("MESOS_NATIVE_LIBRARY")) when it's set, otherwise it will try the System.loadLibrary("mesos");
i.e.
// Tries to load at the specified path.
load(String path) {
if (loaded) return;
ClassLoader contextClassLoader =
Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(
MesosNativeLibrary.class.getClassLoader());
try {
System.load(path);
} catch (UnsatisfiedLinkError error) {
System.err.println("Failed to load native Mesos library at " + path);
throw error;
} finally {
Thread.currentThread().setContextClassLoader(contextClassLoader);
}
}
// Uses MESOS_NATIVE_LIBRARY if set in the environment. Otherwise tries
// to load the "mesos" library.
load() {
if (loaded) return;
// Use MESOS_NATIVE_LIBRARY if specified.
if (System.getenv("MESOS_NATIVE_LIBRARY") != null) {
load(System.getenv("MESOS_NATIVE_LIBRARY"));
return;
}
ClassLoader contextClassLoader =
Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(
MesosNativeLibrary.class.getClassLoader());
try {
System.loadLibrary("mesos");
} catch (UnsatisfiedLinkError error) {
System.err.println("Failed to load native Mesos library from " +
System.getProperty("java.library.path"));
throw error;
} finally {
Thread.currentThread().setContextClassLoader(contextClassLoader);
}
}
I guess the annoyance here is having to duplicate the class loader code, but the functions have a clearer behavior, right now the load(path) method does quite a lot, no?
- Ben Mahler
On July 17, 2013, 1:07 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12618/
> -----------------------------------------------------------
>
> (Updated July 17, 2013, 1:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used this in a subsequent review for Jenkins scheduler.
>
>
> Diffs
> -----
>
> src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 56e97725ae0e94ebd10ce9bc9ba66981a07558fe
>
> Diff: https://reviews.apache.org/r/12618/diff/
>
>
> Testing
> -------
>
> make jenkins
>
>
> Thanks,
>
> Vinod Kone
>
>