On Tue, Oct 18, 2016 at 6:29 PM, Christopher <[email protected]> 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.
https://github.com/apache/incubator-fluo-recipes/pull/115 > > 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 <[email protected]> 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 <[email protected]> wrote: >> >> Keith Turner wrote: >> > On Tue, Oct 18, 2016 at 1:37 PM, Josh Elser<[email protected]> >> 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 :) >> >>
