[GitHub] ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170478654 ## File path: maven-plugin/src/it/plugin-test/pom.xml ## @@ -154,4 +150,58 @@ + + + hadoop-default + + + !hadoop.profile + + + +2 + + + + org.apache.hadoop + hadoop-client + + + + + hadoop2 + + + hadoop.profile + 2 + + + + + org.apache.hadoop + hadoop-client + + + + + hadoop3 + + + hadoop.profile + 3 + + + + + org.apache.hadoop + hadoop-client-api + + + org.apache.hadoop + hadoop-client-runtime + runtime Review comment: It's really too bad Hadoop didn't continue to publish a `hadoop-client` jar which included both of these... it would have been very convenient. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170478991 ## File path: pom.xml ## @@ -899,10 +920,30 @@ java17 1.0 + +org.apache.hadoop.conf.Configuration +org.apache.hadoop.fs.FileSystem + + +enforce-hadoop-profile + + enforce + + + + + hadoop.profile + (2|3) + You should specify the Hadoop profile by major Hadoop generation, i.e. 2 or 3, not by a version number. +Use hadoop.version to use a particular Hadoop version within that generation. + + Review comment: If 3 executions of the enforcer plugin is too slow, we could consider rolling this rule into one of the other executions... probably the basic one. The only reason we have the second execution for the animal sniffer, is because it needs to run after compilation. This rule could run at the beginning with the other basic rules. It's fine separate, too. This is just a thought, in case it's slow. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170478585 ## File path: examples/simple/pom.xml ## @@ -105,4 +101,58 @@ + + + hadoop-default + + + !hadoop.profile + + + +2 Review comment: This isn't hurting anything, but probably is only needed at the top level pom. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170412238 ## File path: pom.xml ## @@ -128,7 +128,9 @@ 3.0.5 1 +2 Review comment: There is a build extension mentioned in the comments of that issue which could be a good workaround to creating a third profile. It provides a mechanism to add a scripted expression to determine if a profile should be active or not. It looks promising, but I'd want to test it in Eclipse M2E (and maybe other IDEs which support Maven) first to see if the correct profile activates by default there also. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411383 ## File path: core/pom.xml ## @@ -282,5 +277,50 @@ + + hadoop2 + + + Review comment: Not sure this comment makes sense without some explanation as to why we were expecting activeByDefault to behave differently than it was documented to behave. Might be better to just say that 'this profile is used when not otherwise specified'. (EDIT: might also want to borrow from previous wording when we had Hadoop 1 and 2 profile) 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the
hadoop3 shaded jars
URL: https://github.com/apache/accumulo/pull/332#discussion_r170411239
##
File path: assemble/bin/bootstrap_config.sh
##
@@ -383,6 +394,25 @@ elif [[ "${HADOOP_VERSION}" == "IOP4.1" ]]; then
sed -e 's///-->/' \
"${CONF_DIR}/$ACCUMULO_SITE" > temp
+ sed -e 's///-->/' \
+ "${CONF_DIR}/$ACCUMULO_SITE" > temp
+ mv temp "${CONF_DIR}/$ACCUMULO_SITE"
+elif [[ "${HADOOP_VERSION}" == "3" ]]; then
+ sed -e 's///-->/' \
+ "${CONF_DIR}/$ACCUMULO_SITE" > temp
+ sed -e 's///-->/' \
+ "${CONF_DIR}/$ACCUMULO_SITE" > temp
Review comment:
This will clobber the contents of temp from the previous sed execution. It's
too bad we didn't just move the file once, and do all the sed calls with the
in-place `-i` flag (but not worth changing in this PR). (EDIT: that is... not
worth changing to use `-i`; the clobbering still needs to be fixed)
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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411152 ## File path: .travis.yml ## @@ -34,4 +34,6 @@ jdk: before_script: - rm -rf $HOME/.m2/repository/org/apache/accumulo - echo "secure-centralhttps://repo.maven.apache.org/maven2central" > $HOME/settings-custom.xml -script: mvn --settings $HOME/settings-custom.xml clean verify javadoc:jar -DskipITs +script: + - mvn --settings $HOME/settings-custom.xml clean verify javadoc:jar -DskipITs + - mvn --settings $HOME/settings-custom.xml clean verify javadoc:jar -DskipITs -Dhadoop.profile=3 Review comment: From the Travis docs, this looks like it will always run both, but fail the build if either task fails. That sounds like what we want... it's just unfortunate that Travis runs these sequentially instead of concurrently, and that you have to scroll down half the page to find where one stops and the other begins. Not much we can do about that, I guess. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411664 ## File path: assemble/pom.xml ## @@ -323,5 +323,67 @@ + + hadoop2 + + + + !hadoop.profile + + + + commons-math3,commons-vfs2,gson,guava,htrace-core,javax.servlet-api,jcommander,jetty-http,jetty-io,jetty-security,jetty-server,jetty-servlet,jetty-util,jline,libthrift,protobuf-java,slf4j-api,slf4j-log4j12 + src/main/assemblies/binary-release.xml + + + + hadoop3 + + + hadoop.profile + 3 + + + + commons-cli,commons-codec,commons-collections,commons-configuration,commons-io,commons-lang,commons-logging,commons-math3,commons-vfs2,gson,guava,htrace-core,htrace-core4,javax.servlet-api,jcommander,jetty-http,jetty-io,jetty-security,jetty-server,jetty-servlet,jetty-util,jline,libthrift,protobuf-java,slf4j-api,slf4j-log4j12 + src/main/assemblies/binary-release-hadoop3.xml + + Review comment: Do these additional bundled jars affect the LICENSE/NOTICE files of our binary tarball in any way? 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411518 ## File path: minicluster/pom.xml ## @@ -154,4 +145,51 @@ + + + hadoop2 + + + + !hadoop.profile + + + + + org.apache.hadoop + hadoop-client + + + org.apache.hadoop + hadoop-minicluster + true + + + + + hadoop3 + + + hadoop.profile + 3 + + + + + org.apache.hadoop + hadoop-client-api + + + org.apache.hadoop + hadoop-client-minicluster + true Review comment: I'm actually very curious whether this optional has any effect, since it's not clear to me how transitive dependency resolution actually happens for dependencies declared in profiles (and we're not generating "effective" POMs based on profiles). It shouldn't hurt, though. I'll make a personal note to investigate this, unless somebody else knows and can educate me. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411608 ## File path: pom.xml ## @@ -128,7 +128,9 @@ 3.0.5 1 +2 Review comment: I think this will actually disable the hadoop2 profile, because of `!hadoop.profile`. Because of https://issues.apache.org/jira/browse/MNG-3328 , when we had a 1 and 2 profile, we had to create a third hadoop default profile which had `!hadoop.profile` and was identical (duplicated) to the hadoop2 profile. It was tedious, but I think it might be the only workaround to MNG-3328. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411271 ## File path: assemble/pom.xml ## @@ -256,6 +234,28 @@ + +org.apache.maven.plugins +maven-dependency-plugin Review comment: Not a huge deal, but this plugin was reordered, making the diff bigger than it needs to be. Might make sense to move it back above exec-maven-plugin. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170411383 ## File path: core/pom.xml ## @@ -282,5 +277,50 @@ + + hadoop2 + + + Review comment: Not sure this comment makes sense without some explanation as to why we were expecting activeByDefault to behave differently than it was documented to behave. Might be better to just say that 'this profile is used when not otherwise specified'. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the
hadoop3 shaded jars
URL: https://github.com/apache/accumulo/pull/332#discussion_r170411239
##
File path: assemble/bin/bootstrap_config.sh
##
@@ -383,6 +394,25 @@ elif [[ "${HADOOP_VERSION}" == "IOP4.1" ]]; then
sed -e 's///-->/' \
"${CONF_DIR}/$ACCUMULO_SITE" > temp
+ sed -e 's///-->/' \
+ "${CONF_DIR}/$ACCUMULO_SITE" > temp
+ mv temp "${CONF_DIR}/$ACCUMULO_SITE"
+elif [[ "${HADOOP_VERSION}" == "3" ]]; then
+ sed -e 's///-->/' \
+ "${CONF_DIR}/$ACCUMULO_SITE" > temp
+ sed -e 's///-->/' \
+ "${CONF_DIR}/$ACCUMULO_SITE" > temp
Review comment:
This will clobber the contents of temp from the previous sed execution. It's
too bad we didn't just move the file once, and do all the sed calls with the
in-place `-i` flag (but not worth changing in this 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] ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170385390 ## File path: server/gc/pom.xml ## @@ -87,4 +83,40 @@ test + + + hadoop2 + + + hadoop.profile + 2 + + + + + org.apache.hadoop + hadoop-client + + + + + hadoop3 + + + hadoop.profile + 3 + + + + + org.apache.hadoop + hadoop-client-api + + + org.apache.hadoop + hadoop-client-runtime Review comment: Should these have scope runtime? I wonder if we're using stuff that's now in this jar... if so, that seems bad and we should avoid that if possible. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170382302 ## File path: assemble/bin/bootstrap_config.sh ## @@ -24,7 +24,7 @@ where options include (long options not available on all platforms): -n, --native Configure to use native libraries -j, --jvmConfigure to use the jvm -o, --overwrite Overwrite the default config directory --v, --versionSpecify the Apache Hadoop version supported versions: '1' '2' +-v, --versionSpecify the Apache Hadoop version supported versions: '1' '2' '3' Review comment: Ha! We don't actually support version 1 anymore. Probably need to remove that from the list. 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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the
hadoop3 shaded jars
URL: https://github.com/apache/accumulo/pull/332#discussion_r170384404
##
File path: assemble/pom.xml
##
@@ -323,5 +283,147 @@
+
+ hadoop2
+
+
+ hadoop.profile
+ 2
+
+
+
+
+
+org.apache.maven.plugins
+maven-dependency-plugin
+
+
+
+generate-dependencies-raw
+
+ list
+
+compile
+
+
${project.build.directory}/dependencies.raw.txt
+ false
+ true
+
+
commons-math3,commons-vfs2,gson,guava,htrace-core,javax.servlet-api,jcommander,jetty-http,jetty-io,jetty-security,jetty-server,jetty-servlet,jetty-util,jline,libthrift,protobuf-java,slf4j-api,slf4j-log4j12
+ true
+
+
+
+
+
+org.apache.maven.plugins
+maven-assembly-plugin
+
+
+binary-assembly
+
+ single
+
+package
+
+
+
src/main/assemblies/binary-release.xml
+
+
+
+
+
+
+
+
+
+ hadoop3
+
+
+ hadoop.profile
+ 3
+
+
+
+
+
+ commons-cli
+ commons-cli
+
+
+ commons-codec
+ commons-codec
+
+
+ commons-collections
+ commons-collections
+
+
+ commons-configuration
+ commons-configuration
+
+
+ commons-io
+ commons-io
+
+
+ commons-lang
+ commons-lang
+
+
+ commons-logging
+ commons-logging
+
+
+ org.apache.htrace
+ htrace-core4
+
+
+
+
+
+org.apache.maven.plugins
+maven-dependency-plugin
+
+
+
+generate-dependencies-raw
Review comment:
Since this task, and the assembly plugin are both executed regardless... but
with different config, I think it would make more sense to put the included
artifact list and the assembly descriptor in a property set by the profile.
That would reduce the redundancy in the plugin configuration.
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 #332: ACCUMULO-4826 Use the hadoop3 shaded jars
ctubbsii commented on a change in pull request #332: ACCUMULO-4826 Use the hadoop3 shaded jars URL: https://github.com/apache/accumulo/pull/332#discussion_r170384652 ## File path: core/pom.xml ## @@ -282,5 +277,50 @@ + + hadoop2 + +true Review comment: These still have the activeByDefault flag, which must be removed, because it does not do what you think. :) 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
