accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Busbey" <s...@manvsbeard.com>
Subject Re: Review Request 23391: ACCUMULO-2986 ease use of rat plugin.
Date Thu, 10 Jul 2014 22:32:15 GMT


> On July 10, 2014, 8:42 p.m., Christopher Tubbs wrote:
> > -1
> > There's absolutely no reason to create a profile and options to skip the execution
of the plugin since there is a built-in mechanism for controlling the rat check's impact on
the build (eg. by setting rat.ignoreErrors). This change unnecessarily complicates the pom
files, with insufficient utility to justify it, because the rat check is relatively fast,
and there's already a mechanism to ignore its results. Even if we were able to come to consensus
on defaulting rat.ignoreErrors to true, I'd still be opposed to the added profiles and property
to control that profile, due to this added complexity with almost no utility.
> 
> Sean Busbey wrote:
>     Even with ignore errors on, the plugin itself running introduces overhead for those
attempting to iterate tightly on e.g. a particular IT or unit test.
>     
>     "Relatively fast" is subjective. I know that both myself and Keith have complained
about the overhead in the past.
> 
> John Vines wrote:
>     Review board is not a place for discussions about whether or not work should be done.
This is a place for analyzing patches, plain and simple. Please take this conversation to
JIRA.
> 
> Christopher Tubbs wrote:
>     FWIW, I just did 'mvn validate' with and without rat defined in the pom, and it was
a total of 3 seconds difference (~7 seconds, vs. ~4 seconds).
>     
>     I'd concede to moving the existing rat plugin moved to a single profile, so that
it could be disabled with "-P '!rat'" (I think that syntax is correct) or with the presence
of a property. That would allow disabling, for tight iterations of tests, but with minimal
added complexity. The other option would be to patch rat upstream to have a proper skip (rather
than only a process-and-ignore) option.
> 
> Sean Busbey wrote:
>     latest patch moves to a single profile.
> 
> Christopher Tubbs wrote:
>     The second patch looks good to me, regarding the profile, but I'm still concerned
about the rat.ignoreErrors=true by default. I think that warrants further discussion on the
mailing list, and I commented on the prior thread. So, I'm +1 to this profile arrangement,
but -1 to rat.ignoreErrors=true. Do you want to split into two patches (the first without
the rat.ignoreErrors property) and apply the first until consensus is reached on that issue?
Because, I wouldn't object to that.

No, I'll wait for the whole patch.


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23391/#review47614
-----------------------------------------------------------


On July 10, 2014, 10:10 p.m., Sean Busbey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23391/
> -----------------------------------------------------------
> 
> (Updated July 10, 2014, 10:10 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2986
>     https://issues.apache.org/jira/browse/ACCUMULO-2986
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> * puts rat plugin into profile
> * activates profile by default
> * ignores rat errrors by default
> 
> 
> Diffs
> -----
> 
>   pom.xml 2bc87cf082dfeb7bfe1a3fac1fe4fba1eaa87edd 
> 
> Diff: https://reviews.apache.org/r/23391/diff/
> 
> 
> Testing
> -------
> 
> verified rat warnings and failures given move from master -> 1.6.1-SNAPSHOT branch
with no definitions, -Drat.skip=true, -Drat.skip=false, -Drat.ignoreErrors=true, and profile
to check errors in ~/.m2/settings.xml
> 
> 
> Thanks,
> 
> Sean Busbey
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message