[GitHub] milleruntime closed pull request #6: ACCUMULO-4718 Add CLASSPATH to accumulo-testing script
milleruntime closed pull request #6: ACCUMULO-4718 Add CLASSPATH to
accumulo-testing script
URL: https://github.com/apache/accumulo-testing/pull/6
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/bin/accumulo-testing b/bin/accumulo-testing
index 84a1d40..3412564 100755
--- a/bin/accumulo-testing
+++ b/bin/accumulo-testing
@@ -70,6 +70,7 @@ fi
function build_shade_jar() {
export
at_shaded_jar="$at_home/core/target/accumulo-testing-core-$at_version-shaded.jar"
+ export CLASSPATH="$at_shaded_jar:$CLASSPATH"
if [ ! -f "$at_shaded_jar" ]; then
echo "Building $at_shaded_jar"
cd "$at_home" || exit 1
@@ -185,7 +186,7 @@
randomwalk_main="org.apache.accumulo.testing.core.randomwalk.Framework"
case "$1" in
ci-createtable)
build_shade_jar
- java -Dlog4j.configuration="file:$log4j_config" -cp "$at_shaded_jar"
org.apache.accumulo.testing.core.continuous.CreateTable "$at_props"
+ java -Dlog4j.configuration="file:$log4j_config"
org.apache.accumulo.testing.core.continuous.CreateTable "$at_props"
;;
ci-local)
if [ -z "$2" ]; then
@@ -195,7 +196,7 @@ ci-local)
fi
determine_app_main "$2"
build_shade_jar
- java -Dlog4j.configuration="file:$log4j_config" -cp "$at_shaded_jar"
"$ci_main" "$at_props"
+ java -Dlog4j.configuration="file:$log4j_config" "$ci_main" "$at_props"
;;
ci-yarn)
if [ -z "$2" ]; then
@@ -229,7 +230,7 @@ rw-local)
exit 1
fi
build_shade_jar
- java -Dlog4j.configuration="file:$log4j_config" -cp "$at_shaded_jar"
"$randomwalk_main" "$at_props" "$2"
+ java -Dlog4j.configuration="file:$log4j_config" "$randomwalk_main"
"$at_props" "$2"
;;
rw-yarn)
if [ -z "$2" ]; then
diff --git a/conf/accumulo-testing-env.sh.example
b/conf/accumulo-testing-env.sh.example
index e03c973..677c23d 100644
--- a/conf/accumulo-testing-env.sh.example
+++ b/conf/accumulo-testing-env.sh.example
@@ -20,6 +20,8 @@ test -z "$HADOOP_PREFIX" && export
HADOOP_PREFIX=/path/to/hadoop
export ACCUMULO_VERSION=`accumulo version`
export HADOOP_VERSION=`hadoop version | head -n1 | awk '{print $2}'`
export ZOOKEEPER_VERSION=3.4.9
+# Make sure Hadoop configuration directory is on the classpath
+export CLASSPATH=$HADOOP_PREFIX/etc/hadoop
# Agitator
#
----
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] jmark99 opened a new pull request #309: ACCUMULO-4722 Use Objects.equals in Pairs class"
jmark99 opened a new pull request #309: ACCUMULO-4722 Use Objects.equals in Pairs class" URL: https://github.com/apache/accumulo/pull/309 ACCUMULO-4722 Use Objects.equals() in Pair equals method Removed private equals method in Pairs class and instead made use of the java.util.Objects.equals method. Updated the test class to verify several variations of the equals method. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime commented on issue #6: ACCUMULO-4718 Add HADOOP_CONF_DIR to rw-local cp
milleruntime commented on issue #6: ACCUMULO-4718 Add HADOOP_CONF_DIR to rw-local cp URL: https://github.com/apache/accumulo-testing/pull/6#issuecomment-336525460 @mikewalch As far as I can tell, I believe my changes in 52baec5 will also satisfy ACCUMULO-4723 It seemed to run fine with the quick test I ran using: accumulo-testing rw-yarn 1 Bulk.xml This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #308: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on issue #308: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/308#issuecomment-336489706 This is closed by 63f5713708900e50b2177405be699cd4d07e97a0 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner closed pull request #308: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner closed pull request #308: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/308 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#issuecomment-336489561 @jmark99 I squashed+merged the changes. There was one diff, I ran mvn compile and it reformatted some stuff. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner closed pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner closed pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner closed pull request #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner closed pull request #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/307 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #6: ACCUMULO-4718 Add HADOOP_CONF_DIR to rw-local cp
ctubbsii commented on a change in pull request #6: ACCUMULO-4718 Add HADOOP_CONF_DIR to rw-local cp URL: https://github.com/apache/accumulo-testing/pull/6#discussion_r144427141 ## File path: bin/accumulo-testing ## @@ -229,7 +229,7 @@ rw-local) exit 1 fi build_shade_jar - java -Dlog4j.configuration="file:$log4j_config" -cp "$at_shaded_jar" "$randomwalk_main" "$at_props" "$2" + java -Dlog4j.configuration="file:$log4j_config" -cp "$at_shaded_jar:$HADOOP_CONF_DIR" "$randomwalk_main" "$at_props" "$2" Review comment: One way we can decouple this from specific environment variables (which may or may not exist), is to simply remove the `-cp` section, and set `CLASSPATH` above the call to `java`. Something like `export CLASSPATH="$at_shaded_jar:$CLASSPATH"` That way, the caller can simply set `CLASSPATH=/path/to/hadoop/confdir` prior to running the `accumulo-testing` command. We could also add a sanity check prior to running java to make sure that the `$CLASSPATH` contains a directory with `core-site.xml` and/or `hdfs-site.xml` (or whatever the Hadoop client configuration uses by default). ---- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#issuecomment-336292989 Yes, @keith-turner , #307 has the correct documentation for 1.8. This documentation is also valid for the current master line of development. These was a slight modification to the loadDefaults() method between 1.7 and 1.8 that necessitated a small change to the documentation. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#issuecomment-336292123 I can merge this. @jmark99 does #307 currently have the changes you want for 1.8? If so I will look at that when merging forward. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 commented on issue #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#issuecomment-336289581 Thanks @keith-turner. I've rendered the javadocs for all versions on my local machine and they render correctly. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 commented on a change in pull request #306: ACCUMULO-4170 Clarify
ClientConfiguration javadocs
URL: https://github.com/apache/accumulo/pull/306#discussion_r144411823
##
File path:
core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##
@@ -199,15 +199,23 @@ public ClientConfiguration(Configuration... configs) {
}
/**
- * Attempts to load a configuration file from the system. Uses the
"ACCUMULO_CLIENT_CONF_PATH" environment variable, split on File.pathSeparator,
for a list
- * of target files. If not set, uses the following in this order-
~/.accumulo/config $ACCUMULO_CONF_DIR/client.conf -OR-
$ACCUMULO_HOME/conf/client.conf
- * (depending on whether $ACCUMULO_CONF_DIR is set) /etc/accumulo/client.conf
+ * Attempts to load a configuration file from the system using the default
search paths. Uses the ACCUMULO_CLIENT_CONF_PATH environment variable,
+ * split on File.pathSeparator, for a list of target files.
+ *
+ * If ACCUMULO_CLIENT_CONF_PATH is not set, uses the following in
this order:
+ *
+ * ~/.accumulo/config
+ * either:
+ *
+ * $ACCUMULO_CONF_DIR/client.conf, if
$ACCUMULO_CONF_DIR is defined.
+ * $ACCUMULO_HOME/conf/client.conf, otherwise.
+ *
+ * /etc/accumulo/client.conf
+ * /etc/accumulo/conf/client.conf
+ *
+ *
*
- * A client configuration will then be read from each location using
PropertiesConfiguration to construct a configuration. That means the latest
item will be
- * the one in the configuration.
- *
- * @see PropertiesConfiguration
- * @see File#pathSeparator
+ * @since 1.6.0
Review comment:
Thanks for the catch! I realized I had inadvertently removed a line of
documentation when I was performing the previous revision.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify
ClientConfiguration javadocs
URL: https://github.com/apache/accumulo/pull/306#discussion_r144407156
##
File path:
core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##
@@ -199,15 +199,23 @@ public ClientConfiguration(Configuration... configs) {
}
/**
- * Attempts to load a configuration file from the system. Uses the
"ACCUMULO_CLIENT_CONF_PATH" environment variable, split on File.pathSeparator,
for a list
- * of target files. If not set, uses the following in this order-
~/.accumulo/config $ACCUMULO_CONF_DIR/client.conf -OR-
$ACCUMULO_HOME/conf/client.conf
- * (depending on whether $ACCUMULO_CONF_DIR is set) /etc/accumulo/client.conf
+ * Attempts to load a configuration file from the system using the default
search paths. Uses the ACCUMULO_CLIENT_CONF_PATH environment variable,
+ * split on File.pathSeparator, for a list of target files.
+ *
+ * If ACCUMULO_CLIENT_CONF_PATH is not set, uses the following in
this order:
+ *
+ * ~/.accumulo/config
+ * either:
+ *
+ * $ACCUMULO_CONF_DIR/client.conf, if
$ACCUMULO_CONF_DIR is defined.
+ * $ACCUMULO_HOME/conf/client.conf, otherwise.
+ *
+ * /etc/accumulo/client.conf
+ * /etc/accumulo/conf/client.conf
+ *
+ *
*
- * A client configuration will then be read from each location using
PropertiesConfiguration to construct a configuration. That means the latest
item will be
- * the one in the configuration.
- *
- * @see PropertiesConfiguration
- * @see File#pathSeparator
+ * @since 1.6.0
Review comment:
This since tag is not needed because its the same as the class level since
tag. Methods inherit the since tag from the class. Only need to set it on
method if it differs from class since tag.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on issue #220: ACCUMULO-4591 replication latency metrics added
ctubbsii commented on issue #220: ACCUMULO-4591 replication latency metrics added URL: https://github.com/apache/accumulo/pull/220#issuecomment-336260108 Closing this, as it was superseded by #305 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii closed pull request #220: ACCUMULO-4591 replication latency metrics added
ctubbsii closed pull request #220: ACCUMULO-4591 replication latency metrics added URL: https://github.com/apache/accumulo/pull/220 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime opened a new pull request #6: ACCUMULO-4718 Add HADOOP_CONF_DIR to rw-local cp
milleruntime opened a new pull request #6: ACCUMULO-4718 Add HADOOP_CONF_DIR to rw-local cp URL: https://github.com/apache/accumulo-testing/pull/6 The test "rw-local Bulk.xml" was throwing errors because it couldn't find the HDFS containing the files to import. The hadoop conf wasn't on the classpath so it was defaulting to the disk "file:///" file system. This doesn't seem to affect the rest of the tests. Not sure if this is the best place to include HADOOP_CONF_DIR... This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] adamjshook commented on issue #220: ACCUMULO-4591 replication latency metrics added
adamjshook commented on issue #220: ACCUMULO-4591 replication latency metrics added URL: https://github.com/apache/accumulo/pull/220#issuecomment-336228003 This PR can be closed with #305 being merged. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#discussion_r144368491 ## File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java ## @@ -41,7 +41,22 @@ /** * Contains a list of property keys recognized by the Accumulo client and convenience methods for setting them. - * + * + * When loading a configuration file from the system, Accumulo uses the ACCUMULO_CLIENT_CONF_PATH environment variable, Review comment: > One for the 1.7 change and another for the 1.8 change and then the 1.8 change will be merged forward to 2.0? So you want the changes in 1.8 to be slightly different? If so it would be nice to end up with the following on 1.8 branch. * Your commit with 1.8 changes * Merge commit with your changes from 1.7 * Your 1.7 commit Lets go for the easiest way to get to those changes. Lets get the changes in 1.7 squared away and then we can get the 1.8 changes figured out. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#discussion_r144362198 ## File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java ## @@ -41,7 +41,22 @@ /** * Contains a list of property keys recognized by the Accumulo client and convenience methods for setting them. - * + * + * When loading a configuration file from the system, Accumulo uses the ACCUMULO_CLIENT_CONF_PATH environment variable, Review comment: Sounds good. I'll update to make the adjustments. So if I understand you correctly I will only need 2 PR's. One for the 1.7 change and another for the 1.8 change and then the 1.8 change will be merged forward to 2.0? Thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#discussion_r144360720 ## File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java ## @@ -41,7 +41,22 @@ /** * Contains a list of property keys recognized by the Accumulo client and convenience methods for setting them. - * + * + * When loading a configuration file from the system, Accumulo uses the ACCUMULO_CLIENT_CONF_PATH environment variable, Review comment: > It can be moved back into the method call only if you think that would be more appropriate. Personally I would put the docs on the loadDefault method and then any constuctors that call load default I would state in the constuctors javadoc that it follows the search behavior described in loadDefatult using s javadoc link. If the docs are at the class level, I think the class level docs would need to detail which constuctors and methods do and/or do not follow the search behavior. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #304: ACCUMULO-4716 Don't cache blks over max array size
jmark99 commented on issue #304: ACCUMULO-4716 Don't cache blks over max array size URL: https://github.com/apache/accumulo/pull/304#issuecomment-336212445 Thanks, Keith. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #304: ACCUMULO-4716 Don't cache blks over max array size
keith-turner commented on issue #304: ACCUMULO-4716 Don't cache blks over max array size URL: https://github.com/apache/accumulo/pull/304#issuecomment-336211364 @jmark99 thanks for the contribution. I squashed these commits and cherry picked them back to 1.7 in commit 6ead4740dbc24485b51d73d0a006d44bb2f52833. I then merged the changes forward in commits ae3aa10027cc0926cd5b319d31bc3fd005ffb50f and 47d13beff82e547019faf551e9bdc76c4429e576. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner closed pull request #304: ACCUMULO-4716 Don't cache blks over max array size
keith-turner closed pull request #304: ACCUMULO-4716 Don't cache blks over max array size URL: https://github.com/apache/accumulo/pull/304 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#discussion_r144357873 ## File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java ## @@ -41,7 +41,22 @@ /** * Contains a list of property keys recognized by the Accumulo client and convenience methods for setting them. - * + * + * When loading a configuration file from the system, Accumulo uses the ACCUMULO_CLIENT_CONF_PATH environment variable, Review comment: Not all the constructors do the search. The described behavior occurs when load configurations use a default load path. Previously the path order was described only in the method. I moved it up to class level based upon the suggestion given by @madrob in the ticket comment. It can be moved back into the method call only if you think that would be more appropriate. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 commented on issue #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/307#issuecomment-336207583 The change for 1.8 is slightly different than for 1.7. Although the 1.8 and 2.0 changes are identical so the #307 could be merged forward. I wasn't sure how that situation was handled so I ended up creating a pull request for master as well. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] asfgit closed pull request #305: [ACCUMULO-4591] Add replication latency metrics
asfgit closed pull request #305: [ACCUMULO-4591] Add replication latency metrics URL: https://github.com/apache/accumulo/pull/305 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on issue #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/307#issuecomment-336189538 Is this the same change as #306? We usually make the change in the oldest branch and then merge it forward. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
keith-turner commented on a change in pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306#discussion_r144339507 ## File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java ## @@ -41,7 +41,22 @@ /** * Contains a list of property keys recognized by the Accumulo client and convenience methods for setting them. - * + * + * When loading a configuration file from the system, Accumulo uses the ACCUMULO_CLIENT_CONF_PATH environment variable, Review comment: Does the behavior described here only happen when calling `loadDefault()`? Or do all of the constuctors do this search? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 opened a new pull request #308: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 opened a new pull request #308: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/308 ACCUMULO-4170 Clarify ClientConfiguration javadocs Updated the javadoc information with the ClientConfiguration class. Specifically reworked the default search path information to be displayed as a list rather than inline, thereby easing readability. Additionally moved the search path list to the class level vice the method level.   This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 opened a new pull request #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 opened a new pull request #307: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/307 ACCUMULO-4170 Clarify ClientConfiguration javadocs Updated the javadoc information with the ClientConfiguration class. Specifically reworked the default search path information to be displayed as a list rather than inline, thereby easing readability. Reworded a few of the sentences and moved the path order up to the class level rather than the method level.   This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 opened a new pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs
jmark99 opened a new pull request #306: ACCUMULO-4170 Clarify ClientConfiguration javadocs URL: https://github.com/apache/accumulo/pull/306 ACCUMULO-4170 Clarify ClientConfiguration javadocs Updated the javadoc information with the ClientConfiguration class. Specifically reworked the default search path information to be displayed as a list rather than inline, thereby easing readability. Reworded a few of the sentences and moved the path order up to the class level rather than the method level.   This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] joshelser commented on issue #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on issue #305: [ACCUMULO-4591] Add replication latency metrics URL: https://github.com/apache/accumulo/pull/305#issuecomment-335938107 No problem, dude. Thanks for the contribution! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] adamjshook commented on issue #305: [ACCUMULO-4591] Add replication latency metrics
adamjshook commented on issue #305: [ACCUMULO-4591] Add replication latency metrics URL: https://github.com/apache/accumulo/pull/305#issuecomment-335930968 @joshelser Thanks, if it wouldn't be too much of a hassle. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] adamjshook commented on issue #305: [ACCUMULO-4591] Add replication latency metrics
adamjshook commented on issue #305: [ACCUMULO-4591] Add replication latency metrics URL: https://github.com/apache/accumulo/pull/305#issuecomment-335895480 @joshelser Back at you! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144061407
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -124,4 +142,55 @@ protected int getNumConfiguredPeers() {
protected int getMaxReplicationThreads() {
return
replicationUtil.getMaxReplicationThreads(master.getMasterMonitorInfo());
}
+
+ protected void addReplicationQueueTimeMetrics() {
+// Exit early if replication table is offline
+if (TableState.ONLINE != Tables.getTableState(master.getInstance(),
ReplicationTable.ID)) {
+ return;
+}
+
+// Exit early if we have no replication peers configured
+if (replicationUtil.getPeers().isEmpty()) {
+ return;
+}
+
+Set paths = replicationUtil.getPendingReplicationPaths();
+
+// We'll take a snap of the current time and use this as a diff between
any deleted
+// file's modification time and now. The reported latency will be off by
at most a
+// number of seconds equal to the metric polling period
+long currentTime = System.currentTimeMillis();
+
+// Iterate through all the pending paths and update the mod time if we
don't know it yet
+for (Path path : paths) {
+ if (!pathModTimes.containsKey(path)) {
+try {
+ pathModTimes.put(path,
master.getFileSystem().getFileStatus(path).getModificationTime());
+} catch (IOException e) {
+ // Ignore all IOExceptions
+ // Either the system is unavailable or the file was deleted
+ // since the initial scan and this check
+}
+ }
+}
+
+// Remove all currently pending files
+Set deletedPaths = new HashSet<>(pathModTimes.keySet());
+deletedPaths.removeAll(paths);
+
+// Exit early if we have no replicated files to report on
+if (deletedPaths.isEmpty()) {
+ return;
+}
+
+replicationQueueTimeStat.resetMinMax();
+
+for (Path path : deletedPaths) {
+ // Remove this path and add the latency
+ long modTime = pathModTimes.remove(path);
Review comment:
It's just the unboxing you really have to worry about. Something like the
following would be fine:
```java
Long modTime = pathModTimes.remove(path);
if (modTime != null) {
...
}
```
Java will handle the unboxing for you once you guaranteed that `modTime` is
definitely non-null.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] adamjshook commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
adamjshook commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144060426
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -124,4 +142,55 @@ protected int getNumConfiguredPeers() {
protected int getMaxReplicationThreads() {
return
replicationUtil.getMaxReplicationThreads(master.getMasterMonitorInfo());
}
+
+ protected void addReplicationQueueTimeMetrics() {
+// Exit early if replication table is offline
+if (TableState.ONLINE != Tables.getTableState(master.getInstance(),
ReplicationTable.ID)) {
+ return;
+}
+
+// Exit early if we have no replication peers configured
+if (replicationUtil.getPeers().isEmpty()) {
+ return;
+}
+
+Set paths = replicationUtil.getPendingReplicationPaths();
+
+// We'll take a snap of the current time and use this as a diff between
any deleted
+// file's modification time and now. The reported latency will be off by
at most a
+// number of seconds equal to the metric polling period
+long currentTime = System.currentTimeMillis();
+
+// Iterate through all the pending paths and update the mod time if we
don't know it yet
+for (Path path : paths) {
+ if (!pathModTimes.containsKey(path)) {
+try {
+ pathModTimes.put(path,
master.getFileSystem().getFileStatus(path).getModificationTime());
+} catch (IOException e) {
+ // Ignore all IOExceptions
+ // Either the system is unavailable or the file was deleted
+ // since the initial scan and this check
+}
+ }
+}
+
+// Remove all currently pending files
+Set deletedPaths = new HashSet<>(pathModTimes.keySet());
+deletedPaths.removeAll(paths);
+
+// Exit early if we have no replicated files to report on
+if (deletedPaths.isEmpty()) {
+ return;
+}
+
+replicationQueueTimeStat.resetMinMax();
+
+for (Path path : deletedPaths) {
+ // Remove this path and add the latency
+ long modTime = pathModTimes.remove(path);
Review comment:
I was actually thinking how best to handle this. It'd be an implementation
error if the path to be removed wasn't in `pathModTimes`, since `deletedPaths`
is a subset of `pathModTimes`. Maybe check for existence and log an error if
it doesn't exist?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144058257
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -124,4 +142,55 @@ protected int getNumConfiguredPeers() {
protected int getMaxReplicationThreads() {
return
replicationUtil.getMaxReplicationThreads(master.getMasterMonitorInfo());
}
+
+ protected void addReplicationQueueTimeMetrics() {
+// Exit early if replication table is offline
+if (TableState.ONLINE != Tables.getTableState(master.getInstance(),
ReplicationTable.ID)) {
+ return;
+}
+
+// Exit early if we have no replication peers configured
+if (replicationUtil.getPeers().isEmpty()) {
+ return;
+}
+
+Set paths = replicationUtil.getPendingReplicationPaths();
+
+// We'll take a snap of the current time and use this as a diff between
any deleted
+// file's modification time and now. The reported latency will be off by
at most a
+// number of seconds equal to the metric polling period
+long currentTime = System.currentTimeMillis();
+
+// Iterate through all the pending paths and update the mod time if we
don't know it yet
+for (Path path : paths) {
+ if (!pathModTimes.containsKey(path)) {
+try {
+ pathModTimes.put(path,
master.getFileSystem().getFileStatus(path).getModificationTime());
+} catch (IOException e) {
+ // Ignore all IOExceptions
+ // Either the system is unavailable or the file was deleted
+ // since the initial scan and this check
+}
+ }
+}
+
+// Remove all currently pending files
+Set deletedPaths = new HashSet<>(pathModTimes.keySet());
+deletedPaths.removeAll(paths);
+
+// Exit early if we have no replicated files to report on
+if (deletedPaths.isEmpty()) {
+ return;
+}
+
+replicationQueueTimeStat.resetMinMax();
+
+for (Path path : deletedPaths) {
+ // Remove this path and add the latency
+ long modTime = pathModTimes.remove(path);
Review comment:
This will throw an error is `pathModTimes` ever does not have a mapping for
`path` because you're unboxing the `Long` into a `long`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144058388
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -124,4 +142,55 @@ protected int getNumConfiguredPeers() {
protected int getMaxReplicationThreads() {
return
replicationUtil.getMaxReplicationThreads(master.getMasterMonitorInfo());
}
+
+ protected void addReplicationQueueTimeMetrics() {
Review comment:
Could you add a simple test case for this, please?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144058516
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -26,38 +29,51 @@
import org.apache.accumulo.master.Master;
import org.apache.accumulo.server.metrics.Metrics;
import org.apache.accumulo.server.replication.ReplicationUtil;
+import org.apache.hadoop.fs.Path;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.metrics2.lib.MutableStat;
/**
*
*/
public class Metrics2ReplicationMetrics implements Metrics, MetricsSource {
public static final String NAME = MASTER_NAME + ",sub=Replication",
DESCRIPTION = "Data-Center Replication Metrics", CONTEXT = "master",
RECORD = "MasterReplication";
- public static final String PENDING_FILES = "filesPendingReplication",
NUM_PEERS = "numPeers", MAX_REPLICATION_THREADS = "maxReplicationThreads";
+ public static final String PENDING_FILES = "filesPendingReplication",
NUM_PEERS = "numPeers", MAX_REPLICATION_THREADS = "maxReplicationThreads",
+ REPLICATION_QUEUE_TIME_QUANTILES = "replicationQueue10m",
REPLICATION_QUEUE_TIME = "replicationQueue";
private final Master master;
private final MetricsSystem system;
private final MetricsRegistry registry;
private final ReplicationUtil replicationUtil;
+ private final MutableQuantiles replicationQueueTimeQuantiles;
+ private final MutableStat replicationQueueTimeStat;
+ private final Map<Path,Long> pathModTimes;
Metrics2ReplicationMetrics(Master master, MetricsSystem system) {
this.master = master;
this.system = system;
+pathModTimes = new HashMap<>();
+
registry = new MetricsRegistry(Interns.info(NAME, DESCRIPTION));
replicationUtil = new ReplicationUtil(master);
+replicationQueueTimeQuantiles =
registry.newQuantiles(REPLICATION_QUEUE_TIME_QUANTILES, "replication queue time
quantiles in milliseconds", "ops",
Review comment:
nit: capitalize "Replication" please
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144057720
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -124,4 +142,55 @@ protected int getNumConfiguredPeers() {
protected int getMaxReplicationThreads() {
return
replicationUtil.getMaxReplicationThreads(master.getMasterMonitorInfo());
}
+
+ protected void addReplicationQueueTimeMetrics() {
+// Exit early if replication table is offline
+if (TableState.ONLINE != Tables.getTableState(master.getInstance(),
ReplicationTable.ID)) {
+ return;
+}
+
+// Exit early if we have no replication peers configured
+if (replicationUtil.getPeers().isEmpty()) {
+ return;
+}
+
+Set paths = replicationUtil.getPendingReplicationPaths();
+
+// We'll take a snap of the current time and use this as a diff between
any deleted
+// file's modification time and now. The reported latency will be off by
at most a
+// number of seconds equal to the metric polling period
+long currentTime = System.currentTimeMillis();
+
+// Iterate through all the pending paths and update the mod time if we
don't know it yet
+for (Path path : paths) {
+ if (!pathModTimes.containsKey(path)) {
+try {
+ pathModTimes.put(path,
master.getFileSystem().getFileStatus(path).getModificationTime());
+} catch (IOException e) {
+ // Ignore all IOExceptions
+ // Either the system is unavailable or the file was deleted
+ // since the initial scan and this check
Review comment:
Add a `log.trace` in case you/someone ever runs into troubles and wants to
try to debug this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add replication latency metrics
joshelser commented on a change in pull request #305: [ACCUMULO-4591] Add
replication latency metrics
URL: https://github.com/apache/accumulo/pull/305#discussion_r144058956
##
File path:
server/master/src/main/java/org/apache/accumulo/master/metrics/Metrics2ReplicationMetrics.java
##
@@ -26,38 +29,51 @@
import org.apache.accumulo.master.Master;
import org.apache.accumulo.server.metrics.Metrics;
import org.apache.accumulo.server.replication.ReplicationUtil;
+import org.apache.hadoop.fs.Path;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsRecordBuilder;
import org.apache.hadoop.metrics2.MetricsSource;
import org.apache.hadoop.metrics2.MetricsSystem;
import org.apache.hadoop.metrics2.lib.Interns;
import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableQuantiles;
+import org.apache.hadoop.metrics2.lib.MutableStat;
/**
*
*/
public class Metrics2ReplicationMetrics implements Metrics, MetricsSource {
public static final String NAME = MASTER_NAME + ",sub=Replication",
DESCRIPTION = "Data-Center Replication Metrics", CONTEXT = "master",
RECORD = "MasterReplication";
- public static final String PENDING_FILES = "filesPendingReplication",
NUM_PEERS = "numPeers", MAX_REPLICATION_THREADS = "maxReplicationThreads";
+ public static final String PENDING_FILES = "filesPendingReplication",
NUM_PEERS = "numPeers", MAX_REPLICATION_THREADS = "maxReplicationThreads",
+ REPLICATION_QUEUE_TIME_QUANTILES = "replicationQueue10m",
REPLICATION_QUEUE_TIME = "replicationQueue";
private final Master master;
private final MetricsSystem system;
private final MetricsRegistry registry;
private final ReplicationUtil replicationUtil;
+ private final MutableQuantiles replicationQueueTimeQuantiles;
+ private final MutableStat replicationQueueTimeStat;
+ private final Map<Path,Long> pathModTimes;
Metrics2ReplicationMetrics(Master master, MetricsSystem system) {
this.master = master;
this.system = system;
+pathModTimes = new HashMap<>();
+
registry = new MetricsRegistry(Interns.info(NAME, DESCRIPTION));
replicationUtil = new ReplicationUtil(master);
+replicationQueueTimeQuantiles =
registry.newQuantiles(REPLICATION_QUEUE_TIME_QUANTILES, "replication queue time
quantiles in milliseconds", "ops",
+"latency", 600);
+replicationQueueTimeStat = registry.newStat(REPLICATION_QUEUE_TIME,
"replication queue time stat in milliseconds", "ops", "latency", true);
Review comment:
nit: "statistics" instead of "stat" (and capitalize Replication too)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] adamjshook opened a new pull request #305: [ACCUMULO-4591] Add replication latency metrics
adamjshook opened a new pull request #305: [ACCUMULO-4591] Add replication latency metrics URL: https://github.com/apache/accumulo/pull/305 Supersedes #220 Trying to capture the latency based on WAL modification time and the current time once the file disappears from the status section of the replication table. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime closed pull request #5: ACCUMULO-4717 Add checkstyle plugin to check API
milleruntime closed pull request #5: ACCUMULO-4717 Add checkstyle plugin to check API URL: https://github.com/apache/accumulo-testing/pull/5 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #304: ACCUMULO-4716 Don't cache blks over max array size
keith-turner commented on a change in pull request #304: ACCUMULO-4716 Don't
cache blks over max array size
URL: https://github.com/apache/accumulo/pull/304#discussion_r143228666
##
File path:
core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/CachableBlockFile.java
##
@@ -156,6 +156,10 @@ public long getStartPos() throws IOException {
private boolean closed = false;
private AccumuloConfiguration accumuloConfiguration = null;
+// ACCUMULO-4716 - Define MAX_ARRAY_SIZE smaller than Integer.MAX_VALUE to
prevent possible OutOfMemory
+// errors when allocating arrays - described in stackoverflow post:
https://stackoverflow.com/a/8381338
Review comment:
I like this comment.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] jmark99 commented on issue #304: ACCUMULO-4716 Don't cache blks over max array size
jmark99 commented on issue #304: ACCUMULO-4716 Don't cache blks over max array size URL: https://github.com/apache/accumulo/pull/304#issuecomment-334795821 ACCUMULO-4716 Don't cache blocks over max array size Modified to move MAX_ARRAY_SIZE from within the method to a constant class value instead. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #304: ACCUMULO-4716 Don't cache blks over max array size
keith-turner commented on a change in pull request #304: ACCUMULO-4716 Don't
cache blks over max array size
URL: https://github.com/apache/accumulo/pull/304#discussion_r143213486
##
File path:
core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/CachableBlockFile.java
##
@@ -342,7 +342,11 @@ private BlockRead getBlock(String _lookup, BlockCache
cache, BlockLoader loader)
private BlockRead cacheBlock(String _lookup, BlockCache cache, BlockReader
_currBlock, String block) throws IOException {
- if ((cache == null) || (_currBlock.getRawSize() > cache.getMaxSize())) {
+ // ACCUMULO-4716 - Define MAX_ARRAY_SIZE smaller than Integer.MAX_VALUE
to prevent possible OutOfMemory
+ // error as described in stackoverflow post:
https://stackoverflow.com/a/8381338.
+ int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8;
Review comment:
Personally, I would make this a static constants because all caps usually
implies that by convention.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] jmark99 opened a new pull request #304: ACCUMULO-4716 Don't cache blks over max array size
jmark99 opened a new pull request #304: ACCUMULO-4716 Don't cache blks over max array size URL: https://github.com/apache/accumulo/pull/304 ACCUMULO-4716 Don't cache blocks over max array size Prevents byte array from caching up to Integer.MAX_VALUE to prevent possible OutofMemory error as described in StackOverflow post https://stackoverflow.com/a/8381338 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime opened a new pull request #5: ACCUMULO-4717 Add checkstyle plugin to check API
milleruntime opened a new pull request #5: ACCUMULO-4717 Add checkstyle plugin to check API URL: https://github.com/apache/accumulo-testing/pull/5 Will only run with "mvn checkstyle:checkstyle" but would be good to check usage of non public API. This is more of a long term goal since there are a lot. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime closed pull request #27: Add FineAndDandy as contributor
milleruntime closed pull request #27: Add FineAndDandy as contributor URL: https://github.com/apache/accumulo-website/pull/27 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] FineAndDandy opened a new pull request #27: Add FineAndDandy as contributor
FineAndDandy opened a new pull request #27: Add FineAndDandy as contributor URL: https://github.com/apache/accumulo-website/pull/27 Add FineAndDandy as Accumulo contributor for PR apache/accumulo#303 and issue ACCUMULO-4713 Thanks! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] FineAndDandy commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
FineAndDandy commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-334448560 Thanks @keith-turner @mjwall I'd love to be added to the people page. I'll open a pr This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mjwall commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
mjwall commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-334290617 too slow, but I reviewed this too and approve. Thanks @FineAndDandy This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner closed pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
keith-turner closed pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
keith-turner commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-334275091 Merged in 0241958ec3cd5e764b94a1fbd485fc859fdc393f Thanks for the contribution @FineAndDandy, nice work. Would you like to be added to our [people page](http://accumulo.apache.org/people/)? If so let me know what info you would like added or open a pr to the [website repo](https://github.com/apache/accumulo-website) to modify `pages/people.md`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
keith-turner commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-334250968 I'm working on merging this. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334236976 Thanks, @keith-turner. I'll take a look at the ticket you provide. On Wed, Oct 4, 2017 at 1:44 PM, Keith Turner <notificati...@github.com> wrote: > @jmark99 <https://github.com/jmark99> I updated the people page. I didn't > include you username because I think those are ASF usernames. If you are > interested I just opened ACCUMULO-4716 > <https://issues.apache.org/jira/browse/ACCUMULO-4716>. Its something I > ran into recently. > > ? > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/apache/accumulo/pull/302#issuecomment-334234886>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AeHFYUQTQ3iMtEQjNJj6DG2WPqtwQyUfks5so8PygaJpZM4PrDna> > . > -------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334234886 @jmark99 I updated the people page. I didn't include you username because I think those are ASF usernames. If you are interested I just opened [ACCUMULO-4716](https://issues.apache.org/jira/browse/ACCUMULO-4716). Its something I ran into recently. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334204287 Keith, currently I've been starting with some of the 'newbie' issues as I'm attempting to come up to speed on the project. If you have any suggestions for tickets that would help me become more familiar with the project internals but wouldn't be too overwhelming initially I would be glad to hear of some. Thanks On Wed, Oct 4, 2017 at 11:53 AM, Keith Turner <notificati...@github.com> wrote: > I look forward to continuing to supply contributions to the project. > > Great! If you would like assistance finding issues to work on let me know. > > ? > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/apache/accumulo/pull/302#issuecomment-334202417>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AeHFYRnPIjXaFNc_hkRQyZdKrf2vlRR0ks5so6oSgaJpZM4PrDna> > . > ---- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334202417 > I look forward to continuing to supply contributions to the project. Great! If you would like assistance finding issues to work on let me know. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334171419 Thanks Keith, Not sure about the email change. No problem though. Yes, I would be interested in being added to the Accumulo people page. I look forward to continuing to supply contributions to the project. You can add the following info if you would: username: jmark99 name: Mark Owens organization: timezone: ET Thanks, mark On Wed, Oct 4, 2017 at 9:56 AM, Keith Turner <notificati...@github.com> wrote: > @jmark99 <https://github.com/jmark99> I merged this through Github and > for some reason it changed your email address in the commit. I'm sorry > about that, not sure why GH did that. > > Thanks for the contribution. Would you like to be added to the Accumulo people > page <http://accumulo.apache.org/people/> as a contributor? If so let me > know what info you would like there and I can add it, or you can make a PR > to the website repo <https://github.com/apache/accumulo-website> to add > yourself. You can just edit pages/people.md > > ? > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/apache/accumulo/pull/302#issuecomment-334163552>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AeHFYWkEVFD5CfiwwJ8NE_13k1H7to3Hks5so46igaJpZM4PrDna> > . > ---- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334163552 @jmark99 I merged this through Github and for some reason it changed your email address in the commit. I'm sorry about that, not sure why GH did that. Thanks for the contribution. Would you like to be added to the Accumulo [people page][1] as a contributor? If so let me know what info you would like there and I can add it, or you can make a PR to the [website repo][2] to add yourself. You can just edit `pages/people.md` [1]: http://accumulo.apache.org/people/ [2]: https://github.com/apache/accumulo-website This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner closed pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner closed pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] FineAndDandy commented on a change in pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
FineAndDandy commented on a change in pull request #303: ACCUMULO-4713:
Correctly set ranges for IteratorUtil.maximizeStartKey?
URL: https://github.com/apache/accumulo/pull/303#discussion_r142669137
##
File path:
core/src/test/java/org/apache/accumulo/core/iterators/system/TimeSettingIteratorTest.java
##
@@ -106,4 +117,135 @@ public void testAvoidKeyCopy() throws Exception {
assertFalse(tsi.hasTop());
}
+ @Test
+ public void testExclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, false);
+it.seek(testRange, new HashSet(), false);
+
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("00".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("11".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertFalse(it.hasTop());
+ }
+
+ @Test
+ public void testInclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, true);
Review comment:
will fix
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] FineAndDandy commented on a change in pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
FineAndDandy commented on a change in pull request #303: ACCUMULO-4713:
Correctly set ranges for IteratorUtil.maximizeStartKey?
URL: https://github.com/apache/accumulo/pull/303#discussion_r142669050
##
File path:
core/src/main/java/org/apache/accumulo/core/iterators/IteratorUtil.java
##
@@ -312,10 +317,15 @@ public static Range maximizeStartKeyTimeStamp(Range
range) {
public static Range minimizeEndKeyTimeStamp(Range range) {
Range seekRange = range;
-if (range.getEndKey() != null && range.getEndKey().getTimestamp() !=
Long.MIN_VALUE) {
+if (range.getEndKey() != null) {
Key seekKey = new Key(seekRange.getEndKey());
Review comment:
will fix
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] FineAndDandy commented on a change in pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
FineAndDandy commented on a change in pull request #303: ACCUMULO-4713:
Correctly set ranges for IteratorUtil.maximizeStartKey?
URL: https://github.com/apache/accumulo/pull/303#discussion_r142668228
##
File path:
core/src/test/java/org/apache/accumulo/core/iterators/system/TimeSettingIteratorTest.java
##
@@ -106,4 +117,135 @@ public void testAvoidKeyCopy() throws Exception {
assertFalse(tsi.hasTop());
}
+ @Test
+ public void testExclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, false);
+it.seek(testRange, new HashSet(), false);
+
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("00".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("11".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertFalse(it.hasTop());
+ }
+
+ @Test
+ public void testInclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, true);
+it.seek(testRange, new HashSet(), false);
+
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("00".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("11".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertFalse(it.hasTop());
+ }
+
+ public class IteratorToSortedKeyValueIterator implements
SortedKeyValueIterator<Key,Value> {
Review comment:
I think I can rework this to use a deleted flag rather than the internal
class for testing. The important part was that I have two keys with the same
Row/CF/CQ/MIN_LONG in a row to reproduce the problem case.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
jmark99 commented on issue #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302#issuecomment-334116454 Thanks for the review, Keith. Comments have been removed and changes pushed to github. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe
TabletServer.majorCompactorDisabled
URL: https://github.com/apache/accumulo/pull/302#discussion_r142532855
##
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##
@@ -1686,7 +1686,9 @@ public synchronized boolean needsSplit() {
// BEGIN PRIVATE METHODS RELATED TO MAJOR COMPACTION
private boolean isCompactionEnabled() {
-return !isClosing() && !getTabletServer().isMajorCompactionDisabled();
+// ACCUMULO-2968: Removed reference to !isMajorCompactionDisabled
in return
Review comment:
I think this comment can be omitted
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe
TabletServer.majorCompactorDisabled
URL: https://github.com/apache/accumulo/pull/302#discussion_r142532990
##
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##
@@ -1925,20 +1924,11 @@ public SplitRunner(Tablet tablet) {
@Override
public void run() {
- if (majorCompactorDisabled) {
-// this will make split task that were queued when shutdown was
-// initiated exit
-return;
- }
-
+ // ACCUMULO-2968: Removed if condition that always returned
false.
Review comment:
I think this comment can be omitted
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe
TabletServer.majorCompactorDisabled
URL: https://github.com/apache/accumulo/pull/302#discussion_r142532833
##
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactionRunner.java
##
@@ -34,12 +34,9 @@ public CompactionRunner(Tablet tablet,
MajorCompactionReason reason) {
@Override
public void run() {
-if (tablet.getTabletServer().isMajorCompactionDisabled()) {
- // this will make compaction tasks that were queued when shutdown was
- // initiated exit
- tablet.removeMajorCompactionQueuedReason(reason);
- return;
-}
+// ACCUMULO-2968: isMajorCompactionDisabled() previously always returned
false.
Review comment:
I think this comment can be omitted
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
keith-turner commented on a change in pull request #302: ACCUMULO-2968: Axe
TabletServer.majorCompactorDisabled
URL: https://github.com/apache/accumulo/pull/302#discussion_r142532884
##
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##
@@ -1963,7 +1953,10 @@ public MajorCompactor(AccumuloConfiguration config) {
@Override
public void run() {
- while (!majorCompactorDisabled) {
+ // ACCUMULO-2968: While loop previously checked for value of
!majorCompactionDisabled.
Review comment:
I think this comment can be omitted
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on issue #300: ACCUMULO-4708 Limit RFile block size to 2GB
keith-turner commented on issue #300: ACCUMULO-4708 Limit RFile block size to 2GB URL: https://github.com/apache/accumulo/pull/300#issuecomment-333985010 I ran the following test against master and it was successful after I made the changes mentioned in the comment at the end of the patch. Based on this it seems that entries large than 2G can currently work. The small changes I made were related to the fact that it seems that MAX_INT-2 is the largest array you can allocate in java. https://gist.github.com/keith-turner/90e318057f55b080dd93aa00277faa4b I was surprised the test with uncompressible data worked. I suspect it created a compressed block larger than 2G which I was thinking would fail. Need to dig and understand why it worked. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on a change in pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
keith-turner commented on a change in pull request #303: ACCUMULO-4713:
Correctly set ranges for IteratorUtil.maximizeStartKey?
URL: https://github.com/apache/accumulo/pull/303#discussion_r142518818
##
File path:
core/src/test/java/org/apache/accumulo/core/iterators/system/TimeSettingIteratorTest.java
##
@@ -106,4 +117,135 @@ public void testAvoidKeyCopy() throws Exception {
assertFalse(tsi.hasTop());
}
+ @Test
+ public void testExclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, false);
+it.seek(testRange, new HashSet(), false);
+
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("00".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("11".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertFalse(it.hasTop());
+ }
+
+ @Test
+ public void testInclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, true);
+it.seek(testRange, new HashSet(), false);
+
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("00".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("11".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertFalse(it.hasTop());
+ }
+
+ public class IteratorToSortedKeyValueIterator implements
SortedKeyValueIterator<Key,Value> {
Review comment:
Why not use the
[SortedMapIterator](https://github.com/apache/accumulo/blob/rel/1.8.1/core/src/main/java/org/apache/accumulo/core/iterators/SortedMapIterator.java)?
It actually does something for seek, which seems like it would be better for
the test?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
keith-turner commented on a change in pull request #303: ACCUMULO-4713:
Correctly set ranges for IteratorUtil.maximizeStartKey?
URL: https://github.com/apache/accumulo/pull/303#discussion_r142524428
##
File path:
core/src/test/java/org/apache/accumulo/core/iterators/system/TimeSettingIteratorTest.java
##
@@ -106,4 +117,135 @@ public void testAvoidKeyCopy() throws Exception {
assertFalse(tsi.hasTop());
}
+ @Test
+ public void testExclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, false);
+it.seek(testRange, new HashSet(), false);
+
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("00".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertTrue(it.hasTop());
+assertTrue(it.getTopValue().equals(new Value("11".getBytes(;
+assertTrue(it.getTopKey().getTimestamp() == 111L);
+it.next();
+assertFalse(it.hasTop());
+ }
+
+ @Test
+ public void testInclusiveEndKey() throws IOException {
+Text row = new Text("a");
+Text colf = new Text("b");
+Text colq = new Text("c");
+Text cv = new Text();
+
+List<Map.Entry<Key,Value>> sources = new ArrayList<>();
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("00".getBytes(;
+sources.add(new AbstractMap.SimpleImmutableEntry<>(new Key(row, colf,
colq, cv, Long.MIN_VALUE), new Value("11".getBytes(;
+
+TimeSettingIterator it = new TimeSettingIterator(new
IteratorToSortedKeyValueIterator(sources.iterator()), 111L);
+IteratorSetting is = new IteratorSetting(1, TimeSettingIterator.class);
+it.init(null, is.getOptions(), null);
+
+Key startKey = new Key();
+Key endKey = new Key(row, colf, colq, cv, Long.MIN_VALUE);
+Range testRange = new Range(startKey, false, endKey, true);
Review comment:
Is the inclusiveness of the end key the only diff with these test? If so,
personally I would combine the code somehow. Could have a function that each
test calls or a single test with a loop like :
```java
for(boolean endKeyInclusive : new boolean[]{false,true}){
//all test code here
}
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
keith-turner commented on a change in pull request #303: ACCUMULO-4713:
Correctly set ranges for IteratorUtil.maximizeStartKey?
URL: https://github.com/apache/accumulo/pull/303#discussion_r142518372
##
File path:
core/src/main/java/org/apache/accumulo/core/iterators/IteratorUtil.java
##
@@ -312,10 +317,15 @@ public static Range maximizeStartKeyTimeStamp(Range
range) {
public static Range minimizeEndKeyTimeStamp(Range range) {
Range seekRange = range;
-if (range.getEndKey() != null && range.getEndKey().getTimestamp() !=
Long.MIN_VALUE) {
+if (range.getEndKey() != null) {
Key seekKey = new Key(seekRange.getEndKey());
Review comment:
This always creates a new Key, but its not always used. Also, in the
following if stmt block it recreates the key and does not used the one created
here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
ctubbsii commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-333978496 @FineAndDandy I was mostly trying to helpfully suggest better log messages for future. We can always clean up log messages when we merge, if we want. I wouldn't worry about it for this one, unless you really want to, in which case a quick `git commit --amend` and force push to your branch would do it. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] FineAndDandy commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
FineAndDandy commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-333961927 No problem. Do you prefer separate commits or should I fix/rebase and push the PR again? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
ctubbsii commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-333949909 Haven't reviewed the change yet (hopefully will have time later), but wanted to immediately suggest to please use shorter subject lines in git log messages. Can provide as much detail as needed in the commit log body, but the subject should be short, or it messes up the readability of tons of git tooling. Here's one (of many) guide which could be used: https://chris.beams.io/posts/git-commit/ A hint: if the first line of the git log message cannot fit into the GitHub PR subject, then it's too long. :) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
ctubbsii commented on issue #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303#issuecomment-333949909 Haven't reviewed the change yet (hopefully will have time later), but wanted to immediately suggest to please use shorter subject lines in git log messages. Can provide as much detail as needed in the commit log body, but the subject should be short, or it messes up the readability of tons of git tooling. Here's one (of many) guide which could be used: https://chris.beams.io/posts/git-commit/ This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] FineAndDandy opened a new pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey?
FineAndDandy opened a new pull request #303: ACCUMULO-4713: Correctly set ranges for IteratorUtil.maximizeStartKey? URL: https://github.com/apache/accumulo/pull/303 ?TimeStamp() and IteratorUtil.minimizeEndKeyTimeStamp() This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] keith-turner commented on issue #300: ACCUMULO-4708 Limit RFile block size to 2GB
keith-turner commented on issue #300: ACCUMULO-4708 Limit RFile block size to 2GB URL: https://github.com/apache/accumulo/pull/300#issuecomment-333680449 I took another in depth look at this change and I am not sure the premise of this fix is correct. Looking at the code it seems that in the case where the key+value exceeds 2G but compresses to less than 2G that it may be ok. I need to do more investigation. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mikewalch closed pull request #6: ACCUMULO-4516: TeraSortIngest splits argument is ignored if less that 10
mikewalch closed pull request #6: ACCUMULO-4516: TeraSortIngest splits argument is ignored if less that 10 URL: https://github.com/apache/accumulo-examples/pull/6 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 opened a new pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled
jmark99 opened a new pull request #302: ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled URL: https://github.com/apache/accumulo/pull/302 ACCUMULO-2968: Axe TabletServer.majorCompactorDisabled Removed references to majorCompatorDisabled variable defined in accumulo/tserver/TabletServer.java. This private variable was defined as 'false' and never modified in source and additionally no means were provided to alter the value in the code. Several locations within TabletServer.java where updated. This included a couple of 'if' statement that would never be executed as well as some return statement where a constant value of true was unnecessarily included. There was one reference each within CompationRunner.java and Tablet.java which were removed also. The change of most note is probably the while loop within the run method of TableServer. Previously the while check read: while(!majorCompatorDisabled) whereas now it reads: while(true)... which was what it would always equate too. This could be seen to be a loss of information perhaps. A comment was added to describe what existed prior to the update. After update all unit tests continued to pass as well as the 'sunny-day' integration tests. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on issue #6: ACCUMULO-4516: TeraSortIngest splits argument is ignored if less that 10
jmark99 commented on issue #6: ACCUMULO-4516: TeraSortIngest splits argument is ignored if less that 10 URL: https://github.com/apache/accumulo-examples/pull/6#issuecomment-333563921 Thanks, Mike On Mon, Oct 2, 2017 at 11:09 AM, Mike Walch <notificati...@github.com> wrote: > *@mikewalch* approved this pull request. > > Thanks for the PR! Looks good to me. I will merge it in soon but I want to > give others a chance to review it first. > > ? > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/accumulo-examples/pull/6#pullrequestreview-66488115>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AeHFYYhid-RQYwYWUXJvHx0Zcj3O_va1ks5soPyOgaJpZM4Pqp81> > . > ---- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 opened a new pull request #6: ACCUMULO-4516: TeraSortIngest splits argumnet is ignored if less that 10
jmark99 opened a new pull request #6: ACCUMULO-4516: TeraSortIngest splits argumnet is ignored if less that 10 URL: https://github.com/apache/accumulo-examples/pull/6 ACCUMULO-4516: TeraSortIngest splits argumnet is ignored if less that 10 arguments are provided. Modified TeraSortIngest.java to remove arg length check as a condition for setting the number of splits. Instead check to see if the splits option is non-zero and if so, set the number of splits. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii closed pull request #301: ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize
ctubbsii closed pull request #301: ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize URL: https://github.com/apache/accumulo/pull/301 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 commented on a change in pull request #301: ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize
jmark99 commented on a change in pull request #301: ACCUMULO-2907 Invalidate
"this may not be applicable for your security setup" warning from initialize
URL: https://github.com/apache/accumulo/pull/301#discussion_r141462552
##
File path:
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##
@@ -644,6 +644,25 @@ private String getRootUserName(Opts opts) throws
IOException {
return rootpass.getBytes(UTF_8);
}
+ /**
+ * Create warning message related to initial password, if appropriate.
+ *
+ * ACCUMULO-2907 Remove unnecessary security warning from console message
unless its actually appropriate.
+ * The warning message should only be displayed when the value of
instance.security.authenticator
+ * differs between the SiteConfiguration and the DefaultConfiguration values.
+ *
+ * @return String containing warning portion of console message.
+ */
+ private String getInitialPasswordWarning() {
+StringBuilder optionalWarning = new StringBuilder();
+Property authenticatorProperty = Property.INSTANCE_SECURITY_AUTHENTICATOR;
+if
(SiteConfiguration.getInstance().get(authenticatorProperty).equals(authenticatorProperty.getDefaultValue()))
+ optionalWarning.append(": ");
+else
+ optionalWarning.append(" (this may not be applicable for your security
setup): ");
+return optionalWarning.toString();
Review comment:
ACCUMULO-2907: Updated to replace StringBuilder usage with String.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #301: ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize
ctubbsii commented on a change in pull request #301: ACCUMULO-2907 Invalidate
"this may not be applicable for your security setup" warning from initialize
URL: https://github.com/apache/accumulo/pull/301#discussion_r141460788
##
File path:
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##
@@ -644,6 +644,25 @@ private String getRootUserName(Opts opts) throws
IOException {
return rootpass.getBytes(UTF_8);
}
+ /**
+ * Create warning message related to initial password, if appropriate.
+ *
+ * ACCUMULO-2907 Remove unnecessary security warning from console message
unless its actually appropriate.
+ * The warning message should only be displayed when the value of
instance.security.authenticator
+ * differs between the SiteConfiguration and the DefaultConfiguration values.
+ *
+ * @return String containing warning portion of console message.
+ */
+ private String getInitialPasswordWarning() {
+StringBuilder optionalWarning = new StringBuilder();
+Property authenticatorProperty = Property.INSTANCE_SECURITY_AUTHENTICATOR;
+if
(SiteConfiguration.getInstance().get(authenticatorProperty).equals(authenticatorProperty.getDefaultValue()))
+ optionalWarning.append(": ");
+else
+ optionalWarning.append(" (this may not be applicable for your security
setup): ");
+return optionalWarning.toString();
Review comment:
Can probably avoid use of StringBuilder here, since there is only a single
string to return in each case, and it's not being concatenated with anything
else until outside the method. Other than that, this change looks good to me.
Thanks for the contribution!
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] jmark99 commented on issue #301: ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize
jmark99 commented on issue #301: ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize URL: https://github.com/apache/accumulo/pull/301#issuecomment-332635247 ACCUMULO-2907: Updated pull request to display security warning message only when applicable. Compare the value of instance.security.authenticator in the Site configuration to the default configuration. If the values differ then the warning message is displayed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mikewalch closed pull request #26: Minor updates to 2.0 docs
mikewalch closed pull request #26: Minor updates to 2.0 docs URL: https://github.com/apache/accumulo-website/pull/26 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mikewalch opened a new pull request #26: Minor updates to 2.0 docs
mikewalch opened a new pull request #26: Minor updates to 2.0 docs URL: https://github.com/apache/accumulo-website/pull/26 * /docs/2.0/ redirects to getting-started/design * Updated layout This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime closed pull request #299: ACCUMULO-4702 Removed Beta Guava dependencies
milleruntime closed pull request #299: ACCUMULO-4702 Removed Beta Guava dependencies URL: https://github.com/apache/accumulo/pull/299 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] milleruntime commented on issue #299: ACCUMULO-4702 Removed Beta Guava dependencies
milleruntime commented on issue #299: ACCUMULO-4702 Removed Beta Guava dependencies URL: https://github.com/apache/accumulo/pull/299#issuecomment-331901805 If there are no objections for this solution to 1.7 I will merge this today. There will need to be follow on work for 1.8 and master. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB
ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile
block size to 2GB
URL: https://github.com/apache/accumulo/pull/300#discussion_r140602218
##
File path:
core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java
##
@@ -2271,4 +2273,40 @@ public void testRootTabletEncryption() throws Exception
{
conf = null;
}
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testKeyTooLarge() throws Exception {
+// If we don't have enough memory (6G) skip these very memory-intensive
unit tests
+Assume.assumeTrue(Runtime.getRuntime().maxMemory() >= 5726797824L);
Review comment:
It's not clear where these numeric literals come from. Perhaps it can be put
in a variable, and their computation shown, so it's clear where they come from?
If they are arbitrary, then a comment explaining that (eg. "arbitrary size in
the range of ... ") would be helpful.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB
ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB URL: https://github.com/apache/accumulo/pull/300#discussion_r140601051 ## File path: pom.xml ## @@ -1548,6 +1548,7 @@ true true true +-Xmx1G Review comment: I think this was intended to be placed in the mail properties section, not the one inside this profile. A simple mistake. :) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB
ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB URL: https://github.com/apache/accumulo/pull/300#discussion_r140601051 ## File path: pom.xml ## @@ -1548,6 +1548,7 @@ true true true +-Xmx1G Review comment: I think this was intended to be placed in the main properties section, not the one inside this profile. A simple mistake. :) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mikewalch closed pull request #25: Simplified documentation
mikewalch closed pull request #25: Simplified documentation URL: https://github.com/apache/accumulo-website/pull/25 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] jmark99 opened a new pull request #301: Accumulo 2907 Invalidate "this may not be applicable for your security setup" warning from initialize
jmark99 opened a new pull request #301: Accumulo 2907 Invalidate "this may not be applicable for your security setup" warning from initialize URL: https://github.com/apache/accumulo/pull/301 ACCUMULO-2907 Invalidate "this may not be applicable for your security setup" warning from initialize Updated console message in the getRootPassword method to remove unnecessary warning about the users security setup. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB
ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile
block size to 2GB
URL: https://github.com/apache/accumulo/pull/300#discussion_r140381442
##
File path:
core/src/test/java/org/apache/accumulo/core/conf/DefaultConfigurationTest.java
##
@@ -51,4 +51,11 @@ public void testGetProperties() {
public void testSanityCheck() {
ConfigSanityCheck.validate(c);
}
+
+ @Test
+ public void testDefaultBlockSizeIsAllowed() {
+long bsi = c.getAsBytes(Property.TABLE_FILE_COMPRESSED_BLOCK_SIZE_INDEX);
+assert (bsi > 0);
+assert (bsi < Integer.MAX_VALUE);
Review comment:
Probably best to avoid assert keyword statements, because assertions can be
disabled in the JVM. Best to use JUnit provided APIs for assertions:
`Assert.assertTrue( expr )`
Also, might be good to add a small one-line comment explaining what we're
checking for. As I understand it, this is mostly just a sanity check on our
default.
We can check more generally that any value in the config is suitable by
putting this check in:
https://github.com/apache/accumulo/blob/master/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java#L46
in the `validate()` method. This gets executed whenever we read various
configurations. We could add a check to ensure that whatever the user has
configured, whichever configuration they've configured it in, that it has a
reasonable value. This is very similar to how we're already checking that the
INSTANCE_ZK_TIMEOUT value is within an allowable range.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile block size to 2GB
ctubbsii commented on a change in pull request #300: ACCUMULO-4708 Limit RFile
block size to 2GB
URL: https://github.com/apache/accumulo/pull/300#discussion_r140379315
##
File path:
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java
##
@@ -287,8 +287,13 @@ public void finish() throws IOException {
*/
public long getRawSize() throws IOException {
/**
- * Expecting the size() of a block not exceeding 4GB. Assuming the
size() will wrap to negative integer if it exceeds 2GB.
+ * size() comes from DataOutputStream, which returns Integer.MAX_VALUE
on an overflow, which means we do not know if 2GB or more data has been written.
+ * Because the data is of an unknown length, we cannot know the block
size. To avoid corrupt RFiles, we throw an exception. This should be addressed
by
+ * whatever object is putting data into the stream to ensure this
condition is never reached.
*/
+if (size() == Integer.MAX_VALUE) {
Review comment:
Just to be safe, could also do `>=` in case implementation changes. We know
it should never be larger than max int, but if implementation starts returning
some larger value (for whatever reason), we don't want to let that through
either.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
