fluo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josh Elser <josh.el...@gmail.com>
Subject Re: [VOTE] Fluo Recipes 1.0.0-incubating-rc0
Date Wed, 19 Oct 2016 15:26:52 GMT
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<ctubbsii@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.elser@gmail.com>  wrote:
>>
>> Keith Turner wrote:
>>> On Tue, Oct 18, 2016 at 1:37 PM, Josh Elser<josh.elser@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 :)
>>
>>
>

Mime
View raw message