Christopher wrote:
-1
* Reviewed signatures, hashes
* Commit matches tarball (with the exception of the generated DEPENDENCIES
file, which is created by the maven-remote-resources-plugin)
* Builds fine with tests, but with a compiler warning about unused
auto-closeable in FluoSparkHelperIT.java which should be fixed
Issues (minor):
* The NOTICE and DISCLAIMER files at the top incorrectly identifies itself
as Apache Fluo, rather than Apache Fluo Recipes. This might be okay with
DISCLAIMER (though, some wording to explain the relationship Apache Fluo
Recipes has to Apache Fluo would help, but it's certainly confusing for
NOTICE.
* The modules don't have the "Apache" branding in their<name> elements in
the pom. This results in NOTICE files that don't have the Apache branding
on the project name, and jar manifests which have the same. Also, the
generated NOTICE files in the jars for the modules look funny "Fluo Recipes
Accumulo". Should we name them more like "Fluo Recipes For Accumulo"?
* We can/should also update the<name> fields and NOTICE files to have "
(incubating)" suffixed at the end.
Good catch on these, Christopher. I hadn't looked at the resulting jars.
Would agree that these should be addressed now if possible.
Other:
* saw weird Implementation-URLs in jar manifests. Not really sure these add
any value... but the URLs certainly shouldn't be expected to exist (they
don't)
On Tue, Oct 18, 2016 at 3:33 PM Christopher<ctubb...@apache.org> wrote:
I'm reviewing this now, but wanted to mention that the branch didn't match
the commit (it was one commit ahead), so I just now pushed a rollback of
that 1 commit to the rc0 branch, and I staged that extra commit to an
rc0-next branch.
On Tue, Oct 18, 2016 at 2:24 PM Josh Elser<josh.el...@gmail.com> wrote:
Keith Turner wrote:
On Tue, Oct 18, 2016 at 1:37 PM, Josh Elser<josh.el...@gmail.com>
wrote:
Keith Turner wrote:
* Saw libthrift in the parent pom.xml, but could not see usages of
that
anywhere in the child modules. (not a release issue, just curious)
Its in the dep mgmt section. The reason is its needed to build
recipes against accumulo 1.8.0. Fluo also uses thrift and depends on
0.9.1. If thrift 0.9.1 ends up on path, then MiniAccumulo will hang.
I can add a comment to the pom.
Ah, ok. I just did a grep and did see any usages of Thrift in the
recipes
which made me wonder why it was defined (even if only in depMgmt).
Overriding Fluo's thrift dependency makes sense -- you don't need to
document on my account.
It took me a bit to remember why it was there. I made a PR to add a
comment to pom.
Ok, sounds great, guys. I just assumed I was being dense and did not
want to demand a comment to be added :)
