You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by mikewalch <gi...@git.apache.org> on 2017/03/03 16:10:27 UTC
[GitHub] accumulo pull request #227: ACCUMULO-4596 Stoped using ACCUMULO_HOME for nat...
GitHub user mikewalch opened a pull request:
https://github.com/apache/accumulo/pull/227
ACCUMULO-4596 Stoped using ACCUMULO_HOME for native libs
* Using system property rather than environment variable to configure
native libraries.
* Removed unnnecessary system property for xml parsing
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikewalch/accumulo native-conf
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/227.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 #227
----
commit 367066f012267a0867afdbc74244e69490d2de19
Author: Mike Walch <mw...@apache.org>
Date: 2017-03-03T16:04:41Z
ACCUMULO-4596 Stoped using ACCUMULO_HOME for native libs
* Using system property rather than environment variable to configure
native libraries.
* Removed unnnecessary system property for xml parsing
----
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104434464
--- Diff: assemble/bin/accumulo-util ---
@@ -411,12 +411,16 @@ function create_config() {
}
function build_native() {
- native_tarball="$basedir/lib/accumulo-native.tar.gz"
final_native_target="$basedir/lib/native"
+ if ls $final_native_target/libaccumulo* &> /dev/null; then
--- End diff --
yes, it is a test operator does not with regex so `ls` must be used but I will change to your suggestion
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104252458
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -60,39 +60,44 @@
* would be a mistake for long lived NativeMaps. Long lived objects are not garbage collected quickly, therefore a process could easily use too much memory.
*
*/
-
public class NativeMap implements Iterable<Map.Entry<Key,Value>> {
private static final Logger log = LoggerFactory.getLogger(NativeMap.class);
private static AtomicBoolean loadedNativeLibraries = new AtomicBoolean(false);
// Load native library
static {
- // Check standard directories
- List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directories set by 'accumulo.native.lib.dirs' system property
+ List<File> directories = new ArrayList<>();
+ String accumuloNativeLibDirs = System.getProperty("accumulo.native.lib.dirs");
+ if (accumuloNativeLibDirs != null) {
+ for (String libDir : accumuloNativeLibDirs.split(":")) {
+ directories.add(new File(libDir));
+ }
}
// Attempt to load from these directories, using standard names
loadNativeLib(directories);
// Check LD_LIBRARY_PATH (DYLD_LIBRARY_PATH on Mac)
if (!isLoaded()) {
+ log.error("Tried and failed to load Accumulo native library from " + accumuloNativeLibDirs);
String ldLibraryPath = System.getProperty("java.library.path");
- String errMsg = "Tried and failed to load native map library from " + ldLibraryPath;
try {
System.loadLibrary("accumulo");
loadedNativeLibraries.set(true);
log.info("Loaded native map shared library from " + ldLibraryPath);
- } catch (Exception e) {
- log.error(errMsg, e);
- } catch (UnsatisfiedLinkError e) {
- log.error(errMsg, e);
+ } catch (Exception | UnsatisfiedLinkError e) {
--- End diff --
multi-catch. Nice :)
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104215216
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
--- End diff --
+1, but noting that "WARN + EXIT" behavior is essentially the definition of a "FATAL" error.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104252311
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -60,39 +60,44 @@
* would be a mistake for long lived NativeMaps. Long lived objects are not garbage collected quickly, therefore a process could easily use too much memory.
*
*/
-
public class NativeMap implements Iterable<Map.Entry<Key,Value>> {
private static final Logger log = LoggerFactory.getLogger(NativeMap.class);
private static AtomicBoolean loadedNativeLibraries = new AtomicBoolean(false);
// Load native library
static {
- // Check standard directories
- List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directories set by 'accumulo.native.lib.dirs' system property
--- End diff --
Small suggestion, for naming consistency: If we're going to support multiple directories, using path syntax and the path separator, we should call this field `accumulo.native.lib.path`
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104253232
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -151,16 +156,13 @@ public static boolean isLoaded() {
private static boolean loadNativeLib(File libFile) {
log.debug("Trying to load native map library " + libFile);
if (libFile.exists() && libFile.isFile()) {
- String errMsg = "Tried and failed to load native map library " + libFile;
try {
System.load(libFile.getAbsolutePath());
loadedNativeLibraries.set(true);
log.info("Loaded native map shared library " + libFile);
return true;
- } catch (Exception e) {
- log.error(errMsg, e);
- } catch (UnsatisfiedLinkError e) {
- log.error(errMsg, e);
+ } catch (Exception | UnsatisfiedLinkError e) {
+ log.error("Tried and failed to load native map library " + libFile, e);
--- End diff --
Should use slf4j substitution syntax instead of string concat.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104250869
--- Diff: assemble/bin/accumulo-util ---
@@ -411,12 +411,16 @@ function create_config() {
}
function build_native() {
- native_tarball="$basedir/lib/accumulo-native.tar.gz"
final_native_target="$basedir/lib/native"
+ if ls $final_native_target/libaccumulo* &> /dev/null; then
+ echo "Accumulo native library already exists in $final_native_target"
+ exit 0
+ fi
+ native_tarball="$basedir/lib/accumulo-native.tar.gz"
if [[ ! -f $native_tarball ]]; then
- echo "Could not find native code artifact: ${native_tarball}";
- exit 1
+ echo "Could not find native code artifact: ${native_tarball}";
--- End diff --
Suggest `1>&2` on the end of the echo.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104250790
--- Diff: assemble/bin/accumulo-util ---
@@ -411,12 +411,16 @@ function create_config() {
}
function build_native() {
- native_tarball="$basedir/lib/accumulo-native.tar.gz"
final_native_target="$basedir/lib/native"
+ if ls $final_native_target/libaccumulo* &> /dev/null; then
--- End diff --
Is `ls` the best way to check if the file exists? If it is, then `libaccumulo.{dylib,jnilib,so,dll}` might be a better check than unconstrained wildcard.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104251445
--- Diff: assemble/conf/templates/accumulo-env.sh ---
@@ -39,12 +41,20 @@ export ZOOKEEPER_HOME="${ZOOKEEPER_HOME:-/path/to/zookeeper}"
##################################################################
## JVM options set for all processes. Extra options can be passed in by setting ACCUMULO_JAVA_OPTS to an array of options.
-JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}" '-XX:+UseConcMarkSweepGC' '-XX:CMSInitiatingOccupancyFraction=75' '-XX:+CMSClassUnloadingEnabled'
-'-XX:OnOutOfMemoryError=kill -9 %p' '-XX:-OmitStackTraceInFastThrow' '-Djava.net.preferIPv4Stack=true'
-'-Djavax.xml.parsers.DocumentBuilderFactory=com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl')
+JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}"
+ '-XX:+UseConcMarkSweepGC'
+ '-XX:CMSInitiatingOccupancyFraction=75'
+ '-XX:+CMSClassUnloadingEnabled'
+ '-XX:OnOutOfMemoryError=kill -9 %p'
+ '-XX:-OmitStackTraceInFastThrow'
+ '-Djava.net.preferIPv4Stack=true'
+ "-Daccumulo.native.lib.dirs=${lib}/native")
+
+## Make sure Accumulo native libraries are built as they are enabled by default
--- End diff --
s/built as they/built, since they/
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104456123
--- Diff: assemble/bin/accumulo-util ---
@@ -571,20 +570,17 @@ function load_jars_hdfs() {
"$HADOOP_PREFIX/bin/hadoop" fs -rm "$SYSTEM_CONTEXT_HDFS_DIR/slf4j*.jar" > /dev/null
for f in $(grep -v '^#' "${conf}/tservers")
do
- rsync -ra --delete "$ACCUMULO_HOME" "$(dirname "$ACCUMULO_HOME")"
+ rsync -ra --delete "$basedir" "$(dirname "$basedir")"
--- End diff --
Created https://issues.apache.org/jira/browse/ACCUMULO-4598
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104197732
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
--- End diff --
Seems pretty simple to support the old mechanism when the new system property isn't defined. Even with this being a 2.0 thing, I think this is something that just has the potential to cause headaches.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/227
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104252836
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -60,39 +60,44 @@
* would be a mistake for long lived NativeMaps. Long lived objects are not garbage collected quickly, therefore a process could easily use too much memory.
*
*/
-
public class NativeMap implements Iterable<Map.Entry<Key,Value>> {
private static final Logger log = LoggerFactory.getLogger(NativeMap.class);
private static AtomicBoolean loadedNativeLibraries = new AtomicBoolean(false);
// Load native library
static {
- // Check standard directories
- List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directories set by 'accumulo.native.lib.dirs' system property
+ List<File> directories = new ArrayList<>();
+ String accumuloNativeLibDirs = System.getProperty("accumulo.native.lib.dirs");
+ if (accumuloNativeLibDirs != null) {
+ for (String libDir : accumuloNativeLibDirs.split(":")) {
+ directories.add(new File(libDir));
+ }
}
// Attempt to load from these directories, using standard names
loadNativeLib(directories);
// Check LD_LIBRARY_PATH (DYLD_LIBRARY_PATH on Mac)
if (!isLoaded()) {
+ log.error("Tried and failed to load Accumulo native library from " + accumuloNativeLibDirs);
String ldLibraryPath = System.getProperty("java.library.path");
- String errMsg = "Tried and failed to load native map library from " + ldLibraryPath;
try {
System.loadLibrary("accumulo");
loadedNativeLibraries.set(true);
--- End diff --
Really not sure we need this fall-back, but I guess it's not going to hurt to leave it in for now.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104253112
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -60,39 +60,44 @@
* would be a mistake for long lived NativeMaps. Long lived objects are not garbage collected quickly, therefore a process could easily use too much memory.
*
*/
-
public class NativeMap implements Iterable<Map.Entry<Key,Value>> {
private static final Logger log = LoggerFactory.getLogger(NativeMap.class);
private static AtomicBoolean loadedNativeLibraries = new AtomicBoolean(false);
// Load native library
static {
- // Check standard directories
- List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directories set by 'accumulo.native.lib.dirs' system property
+ List<File> directories = new ArrayList<>();
+ String accumuloNativeLibDirs = System.getProperty("accumulo.native.lib.dirs");
+ if (accumuloNativeLibDirs != null) {
+ for (String libDir : accumuloNativeLibDirs.split(":")) {
+ directories.add(new File(libDir));
+ }
}
// Attempt to load from these directories, using standard names
loadNativeLib(directories);
// Check LD_LIBRARY_PATH (DYLD_LIBRARY_PATH on Mac)
if (!isLoaded()) {
+ log.error("Tried and failed to load Accumulo native library from " + accumuloNativeLibDirs);
String ldLibraryPath = System.getProperty("java.library.path");
- String errMsg = "Tried and failed to load native map library from " + ldLibraryPath;
try {
System.loadLibrary("accumulo");
loadedNativeLibraries.set(true);
log.info("Loaded native map shared library from " + ldLibraryPath);
- } catch (Exception e) {
- log.error(errMsg, e);
- } catch (UnsatisfiedLinkError e) {
- log.error(errMsg, e);
+ } catch (Exception | UnsatisfiedLinkError e) {
+ log.error("Tried and failed to load Accumulo native library from " + ldLibraryPath, e);
}
}
+
+ // Exit if native libraries could not be loaded
+ if (!isLoaded()) {
+ log.error("Exiting as Accumulo native libraries were requested but could not be be loaded. Either set '"
+ + Property.TSERV_NATIVEMAP_ENABLED
+ + "' to false in accumulo-site.xml or make sure native libraries are created in directories set by the JVM system property 'accumulo.native.lib.dirs' in accumulo-env.sh!");
+ System.exit(-1);
--- End diff --
Shouldn't exit with negative codes. They often have special meanings or are reserved. They're also not cross-platform. This is just a general best-practice, although I'm sure we probably violate it elsewhere, too.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104196514
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
--- End diff --
Thinking we should no longer look in these standard directories. Only look in the configured path. These could possibly be configured as defaults `accumulo-env.sh`, but should not be hard coded.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104252001
--- Diff: assemble/conf/templates/accumulo-env.sh ---
@@ -72,14 +82,13 @@ export JAVA_OPTS
############################
export MALLOC_ARENA_MAX=${MALLOC_ARENA_MAX:-1}
+## Add Hadoop native libraries to shared library paths for Linux & Mac
+export LD_LIBRARY_PATH="${HADOOP_PREFIX}/lib/native:${LD_LIBRARY_PATH}" # For Linux
--- End diff --
Could put this in a conditional:
```bash
case "$(uname)" in
Darwin)
export DYLD_LIBRARY_PATH="${HADOOP_PREFIX}/lib/native:${DYLD_LIBRARY_PATH}"
;;
*)
export LD_LIBRARY_PATH="${HADOOP_PREFIX}/lib/native:${LD_LIBRARY_PATH}"
;;
esac
```
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104209644
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directory set by 'accumulo.native.lib.dir' system property
+ String accumuloNativeLibDir = System.getProperty("accumulo.native.lib.dir");
+ if (accumuloNativeLibDir != null) {
+ directories.add(new File(accumuloNativeLibDir));
--- End diff --
I don't think we should be using this multiple directories strategy at all, nor the System.loadLibrary strategy below. It's too complex, confusing, and prone to problems (thinking in particular multi-arch issues with System.loadLibary), and the fact that "standard library directories" are so platform specific.
We should just do:
```
if native == true:
load native map from file specified by prop (or dir from prop + filename from System.mapLibraryName)
if load failed:
exit process
```
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104210324
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
--- End diff --
I think we should limit the ways users can set native libraries are loaded but fail if native maps are enabled and the libraries cannot be loaded. Something like below:
1. Check if `tserver.memory.maps.native.enabled` is true in `accumulo-site.xml`. If false, do nothing.
2. If true, check if the system property `accumulo.native.lib.dir` is set. If not, warn and exit.
3. If the property is set, try to load native library and warn/exit if library cannot be loaded.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104454669
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -151,16 +156,13 @@ public static boolean isLoaded() {
private static boolean loadNativeLib(File libFile) {
log.debug("Trying to load native map library " + libFile);
if (libFile.exists() && libFile.isFile()) {
- String errMsg = "Tried and failed to load native map library " + libFile;
try {
System.load(libFile.getAbsolutePath());
loadedNativeLibraries.set(true);
log.info("Loaded native map shared library " + libFile);
return true;
- } catch (Exception e) {
- log.error(errMsg, e);
- } catch (UnsatisfiedLinkError e) {
- log.error(errMsg, e);
+ } catch (Exception | UnsatisfiedLinkError e) {
+ log.error("Tried and failed to load native map library " + libFile, e);
--- End diff --
Fixed
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104201582
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
--- End diff --
One possible overall strategy with all of these script changes is the following :
* Use completely new environment var names in the 2.0 scripts
* If 1.X env var names are seen, print a warning with a helpful message.
---
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] accumulo issue #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for native li...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/227
I am going to squash and merge today if no one objects.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104454813
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -60,39 +60,44 @@
* would be a mistake for long lived NativeMaps. Long lived objects are not garbage collected quickly, therefore a process could easily use too much memory.
*
*/
-
public class NativeMap implements Iterable<Map.Entry<Key,Value>> {
private static final Logger log = LoggerFactory.getLogger(NativeMap.class);
private static AtomicBoolean loadedNativeLibraries = new AtomicBoolean(false);
// Load native library
static {
- // Check standard directories
- List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directories set by 'accumulo.native.lib.dirs' system property
+ List<File> directories = new ArrayList<>();
+ String accumuloNativeLibDirs = System.getProperty("accumulo.native.lib.dirs");
+ if (accumuloNativeLibDirs != null) {
+ for (String libDir : accumuloNativeLibDirs.split(":")) {
+ directories.add(new File(libDir));
+ }
}
// Attempt to load from these directories, using standard names
loadNativeLib(directories);
// Check LD_LIBRARY_PATH (DYLD_LIBRARY_PATH on Mac)
if (!isLoaded()) {
+ log.error("Tried and failed to load Accumulo native library from " + accumuloNativeLibDirs);
String ldLibraryPath = System.getProperty("java.library.path");
- String errMsg = "Tried and failed to load native map library from " + ldLibraryPath;
try {
System.loadLibrary("accumulo");
loadedNativeLibraries.set(true);
--- End diff --
I think it can be removed too but it can't hurt to have it in
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104198498
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
--- End diff --
At the very least, if it nots supported. This could check if `ACCUMULO_HOME` is set and print a warning.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104438882
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -60,39 +60,44 @@
* would be a mistake for long lived NativeMaps. Long lived objects are not garbage collected quickly, therefore a process could easily use too much memory.
*
*/
-
public class NativeMap implements Iterable<Map.Entry<Key,Value>> {
private static final Logger log = LoggerFactory.getLogger(NativeMap.class);
private static AtomicBoolean loadedNativeLibraries = new AtomicBoolean(false);
// Load native library
static {
- // Check standard directories
- List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
+ // Check in directories set by 'accumulo.native.lib.dirs' system property
+ List<File> directories = new ArrayList<>();
+ String accumuloNativeLibDirs = System.getProperty("accumulo.native.lib.dirs");
+ if (accumuloNativeLibDirs != null) {
+ for (String libDir : accumuloNativeLibDirs.split(":")) {
+ directories.add(new File(libDir));
+ }
}
// Attempt to load from these directories, using standard names
loadNativeLib(directories);
// Check LD_LIBRARY_PATH (DYLD_LIBRARY_PATH on Mac)
if (!isLoaded()) {
+ log.error("Tried and failed to load Accumulo native library from " + accumuloNativeLibDirs);
String ldLibraryPath = System.getProperty("java.library.path");
- String errMsg = "Tried and failed to load native map library from " + ldLibraryPath;
try {
System.loadLibrary("accumulo");
loadedNativeLibraries.set(true);
log.info("Loaded native map shared library from " + ldLibraryPath);
- } catch (Exception e) {
- log.error(errMsg, e);
- } catch (UnsatisfiedLinkError e) {
- log.error(errMsg, e);
+ } catch (Exception | UnsatisfiedLinkError e) {
+ log.error("Tried and failed to load Accumulo native library from " + ldLibraryPath, e);
}
}
+
+ // Exit if native libraries could not be loaded
+ if (!isLoaded()) {
+ log.error("Exiting as Accumulo native libraries were requested but could not be be loaded. Either set '"
+ + Property.TSERV_NATIVEMAP_ENABLED
+ + "' to false in accumulo-site.xml or make sure native libraries are created in directories set by the JVM system property 'accumulo.native.lib.dirs' in accumulo-env.sh!");
+ System.exit(-1);
--- End diff --
Will change
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104434490
--- Diff: assemble/bin/accumulo-util ---
@@ -411,12 +411,16 @@ function create_config() {
}
function build_native() {
- native_tarball="$basedir/lib/accumulo-native.tar.gz"
final_native_target="$basedir/lib/native"
+ if ls $final_native_target/libaccumulo* &> /dev/null; then
+ echo "Accumulo native library already exists in $final_native_target"
+ exit 0
+ fi
+ native_tarball="$basedir/lib/accumulo-native.tar.gz"
if [[ ! -f $native_tarball ]]; then
- echo "Could not find native code artifact: ${native_tarball}";
- exit 1
+ echo "Could not find native code artifact: ${native_tarball}";
--- End diff --
will add
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104197904
--- Diff: assemble/conf/templates/accumulo-env.sh ---
@@ -39,9 +39,14 @@ export ZOOKEEPER_HOME="${ZOOKEEPER_HOME:-/path/to/zookeeper}"
##################################################################
## JVM options set for all processes. Extra options can be passed in by setting ACCUMULO_JAVA_OPTS to an array of options.
-JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}" '-XX:+UseConcMarkSweepGC' '-XX:CMSInitiatingOccupancyFraction=75' '-XX:+CMSClassUnloadingEnabled'
-'-XX:OnOutOfMemoryError=kill -9 %p' '-XX:-OmitStackTraceInFastThrow' '-Djava.net.preferIPv4Stack=true'
-'-Djavax.xml.parsers.DocumentBuilderFactory=com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl')
+JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}"
+ '-XX:+UseConcMarkSweepGC'
+ '-XX:CMSInitiatingOccupancyFraction=75'
+ '-XX:+CMSClassUnloadingEnabled'
+ '-XX:OnOutOfMemoryError=kill -9 %p'
+ '-XX:-OmitStackTraceInFastThrow'
+ '-Djava.net.preferIPv4Stack=true'
+ "-Daccumulo.native.lib.dir=${ACCUMULO_HOME}/lib/native")
--- End diff --
Could make this property a list of dirs, instead of a single dir. The comment about the standard directories made me think of `-Daccumulo.native.lib.dirs=${ACCUMULO_HOME}/lib/native:/usr/lib64:/usr/lib`
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104250990
--- Diff: assemble/bin/accumulo-util ---
@@ -512,13 +516,8 @@ function gen_monitor_cert() {
}
function load_jars_hdfs() {
- export ACCUMULO_HOME="$basedir"
-
- if [ -f "${conf}/accumulo-env.sh" ]; then
- source "$conf/accumulo-env.sh"
- fi
if [ -z "$HADOOP_PREFIX" ]; then
- echo "HADOOP_PREFIX is not set. Please make sure it's set globally or in $conf/accumulo-env.sh"
+ echo "HADOOP_PREFIX is not set!"
--- End diff --
Will be nice when we get to a point where we don't actually require this any more.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104251241
--- Diff: assemble/bin/accumulo-util ---
@@ -571,20 +570,17 @@ function load_jars_hdfs() {
"$HADOOP_PREFIX/bin/hadoop" fs -rm "$SYSTEM_CONTEXT_HDFS_DIR/slf4j*.jar" > /dev/null
for f in $(grep -v '^#' "${conf}/tservers")
do
- rsync -ra --delete "$ACCUMULO_HOME" "$(dirname "$ACCUMULO_HOME")"
+ rsync -ra --delete "$basedir" "$(dirname "$basedir")"
--- End diff --
This rsync command is currently broken, and has been for some time. Probably should just remove it. I know you're just updating variable names here, but thought I'd mention it, in case you want to clean it up in a follow-on issue.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104201693
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java ---
@@ -70,11 +70,10 @@
static {
// Check standard directories
List<File> directories = new ArrayList<>(Arrays.asList(new File[] {new File("/usr/lib64"), new File("/usr/lib")}));
- // Check in ACCUMULO_HOME location, too
- String accumuloHome = System.getenv("ACCUMULO_HOME");
- if (accumuloHome != null) {
- directories.add(new File(accumuloHome + "/lib/native"));
- directories.add(new File(accumuloHome + "/lib/native/map")); // old location, just in case somebody puts it here
--- End diff --
I think checking for the property, then checking for the ACCUMULO_HOME, then checking doing something in the absence of either of them... at some point, the number of layers makes it hard to grok and is no longer "simple".
If we want simplicity, a better way is to provide good defaults that are easily modified in the configs, rather than trying to handle a bunch of various conditions in code.
---
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] accumulo pull request #227: ACCUMULO-4596 Stopped using ACCUMULO_HOME for na...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/227#discussion_r104209937
--- Diff: assemble/conf/templates/accumulo-env.sh ---
@@ -39,9 +39,14 @@ export ZOOKEEPER_HOME="${ZOOKEEPER_HOME:-/path/to/zookeeper}"
##################################################################
## JVM options set for all processes. Extra options can be passed in by setting ACCUMULO_JAVA_OPTS to an array of options.
-JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}" '-XX:+UseConcMarkSweepGC' '-XX:CMSInitiatingOccupancyFraction=75' '-XX:+CMSClassUnloadingEnabled'
-'-XX:OnOutOfMemoryError=kill -9 %p' '-XX:-OmitStackTraceInFastThrow' '-Djava.net.preferIPv4Stack=true'
-'-Djavax.xml.parsers.DocumentBuilderFactory=com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl')
+JAVA_OPTS=("${ACCUMULO_JAVA_OPTS[@]}"
+ '-XX:+UseConcMarkSweepGC'
+ '-XX:CMSInitiatingOccupancyFraction=75'
+ '-XX:+CMSClassUnloadingEnabled'
+ '-XX:OnOutOfMemoryError=kill -9 %p'
+ '-XX:-OmitStackTraceInFastThrow'
+ '-Djava.net.preferIPv4Stack=true'
+ "-Daccumulo.native.lib.dir=${ACCUMULO_HOME}/lib/native")
--- End diff --
Or just a list of native libraries to load on startup. That'd make it easier to load Hadoop native libs, too.
---
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.
---