> 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.
Sean - What is your +1 scenario? Keith - I am specifically using the storeless statistics, which only keep the running total. - Mike ----------------------------------------------------------- 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 > >
