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 19368: ACCUMULO-2494 Delegate math to commons-math
Date Wed, 19 Mar 2014 15:17:06 GMT


> On March 18, 2014, 7:39 p.m., Sean Busbey wrote:
> > which branch is this against?
> 
> Mike Drob wrote:
>     Probably should be against 1.5.1, but I'm not sure.
> 
> Sean Busbey wrote:
>     I'd like to see this against all active dev branches, starting with 1.4.5-SNAP. I
can file a follow on backport ticket if you like.
> 
> kturner wrote:
>     in 1.6.0-SNAPSHOT, the function to get the std dev seems to only be used in test
code.   If this is the case in earlier relases, then thats not a very strong case for applying
it.
> 
> Josh Elser wrote:
>     Please, let's avoid backports for new changes that come in. Put in the correct place
the first time.
> 
> Sean Busbey wrote:
>     yeah, I'd prefer that this start in 1.4.5 and then get brought forward to master.
>     
>     Keith, I get the desire to avoid maintenance but I think having known incorrect behavior
with a compatible fix in versions we haven't EOLed yet is going to cause more problems going
forward. Doubly so if the error is in something our tests are built upon.
> 
> kturner wrote:
>     Not trying to avoid maintenance, just thinking about risk and benefits.  I have seen
multiple times in the past where small seemingly innocuous changes for minor bugs have introduced
critical bugs.  In this case TabletServer uses the Stat class, but does not use the std deviation.
 The risk is a small possibility of introducing a new critical bug in tserver if the change
breaks Stat in some strange new way.  The benefit of the change is that informational output
from a few test may be better.
> 
> Mike Drob wrote:
>     The only thing I can think of, is if there is some strange performance-related issue
that comes out of switching to using commons-math. Since 1.4 does not yet depend on commons-math,
I don't want to introduce that. However, since 1.5 does, we could fix retrofit the stdev function
to use it there, and then replace the full implementation in 1.6 and newer? or swap out the
full implementation in just master? Thoughts on this proposed compromise?
> 
> Sean Busbey wrote:
>     My concern with doing nothing is if code that's still under development changes in
the future to rely on Stat's stddev implementation. I'm reasonably confident that commons-math
is mature and this patch adds tests to make sure Stats itself is behaving as expected. Given
Keith's concerns, I'd be happy with leaving the fix out of older versions so long as we put
a known issue note on Stat that points to ACCUMULO-2494.
>     
>     Mike, your compromise sounds like we're just setting ourselves up for a more complicated
maintenance tail. I agree that switching to commons-math is the best choice going forward.
If the risk for regressions is too great to balance out the improvement, I don't think we
should get into how much of the patch meets a risk-reward threshold in the older branches.
>     
>     -1 on partial retrofitting, it complicates maintenance by splitting the patch without
substantial benefit
>     -0 on just a note of issue for 1.4 and 1.5, full implementation swap in 1.6+
>     +0 on just a note of issue for 1.4, full implementation swap in 1.5+
> 
> kturner wrote:
>     Something else that would cause problems would be if the Apache library stored all
of the data you were computing stats for.  I assume it does not do this, but would have to
inspect the code to be sure.  
>     
>     I think the more non-critical bug fixes we make to 1.4 we increase the chance that
one of those will introduce a new critical bug.
> 
> Mike Drob wrote:
>     Sean - What is your +1 scenario?
>     Keith - I am specifically using the storeless statistics, which only keep the running
total.

Keith, yeah I agree. I think the only question now is if 1.5 gets included.

Do we already have an integration test that stresses Stat enough to surface any issues with
stored state?

Mike, I'm +1 overall on the patch still. that third option probably should have reflected
it with a +1 instead of +0.


- Sean


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


On March 18, 2014, 8:40 p.m., Mike Drob wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19368/
> -----------------------------------------------------------
> 
> (Updated March 18, 2014, 8:40 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2494
>     https://issues.apache.org/jira/browse/ACCUMULO-2494
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-2494 Delegate math to commons-math
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/util/Stat.java e65265c6decde47ef229377653112a677fef8112

>   core/src/test/java/org/apache/accumulo/core/util/StatTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19368/diff/
> 
> 
> Testing
> -------
> 
> Unit tests results compared with calculations from Wolfram Alpha.
> 
> 
> Thanks,
> 
> Mike Drob
> 
>


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