Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#782 closed task (fixed)

Refactor some rate calculations into net.i2p.stat.Rate

Reported by: Zlatin Balevsky Owned by: Zlatin Balevsky
Priority: trivial Milestone: 0.9.4
Component: api/general Version: 0.9.3
Keywords: refactoring Cc: Zlatin Balevsky
Parent Tickets: Sensitive: no

Description

Nov 15 11:39:32 <zzz> topiltzin_, that paradigm in RouterThrottleImpl (take the average of current and last interval, and if no events take the lifetime average) is widely used and unsynced everywhere
Nov 15 11:39:50 <zzz> it would be an improvement to move that code to a new synced method in Rate.java
Nov 15 11:40:03 <zzz> and maybe some other common calculations on Rates also.

Subtickets

Change History (7)

comment:1 Changed 7 years ago by Zlatin Balevsky

work happening in branch i2p.i2p.zab.782 (notice a pattern?)

comment:2 Changed 7 years ago by zzz

Component: router/generalapi/general
Milestone: 0.9.4

Nice. Code looks fine.

Docs:

  • @since 0.9.4 on the new method and class please
  • Some minimal javadocs on the getters please? (average is a weighted average of current and last, unless both have 0 events, then lifetime…. etc.)

Also:

  • I usually comment out the main() code instead of deleting it, as it may be useful to a future unit test writer… If we already have a unit test for Rate, then fine, otherwise better to either leave it in or move it to a new unit test.

Thanks for asking me to look at it.

comment:3 Changed 7 years ago by Zlatin Balevsky

  • Javadocs added
  • setters made package-private
  • existing JUnit test in net.i2p.stat.RateTest is identical to the main() method I'm removing.

Please re-review head of branch before I merge.

comment:4 Changed 7 years ago by zzz

Sorry, I meant Rate.computeAverages() and getAvgOrLifetimeAvg() need an @since. The RateAverages? getters don't need an @since b/c the whole class has an @since. But leave them there if you like.

If lastEventCount is 0 then getAverageValue() will always be 0, right? The RateAverages?.getAverage() javadoc says it returns the current average, but the computeAverages() code returns getAverageValue() which is the *last* average, but it doesn't matter because both are zero. So can't the code and the javadocs be clarified slightly for this case?

These are all nits. Thanks again for asking for review. Feel free to merge.

comment:5 Changed 7 years ago by Zlatin Balevsky

Resolution: fixed
Status: newclosed

I'm not sure that's right : if both last and current event counts are zero is Rate.getAverageValue == RaveAverages.getValue().

If only lastEventCount is zero, AND the useLifetimeAverage parameter == false is RateAverages.getValue() == 0. lifetimeEventCount can be > 0 while lastEventCount == 0 before the first coalescing.

I've updated the javadocs accordingly and merged with a2b59616f0810a0f735d18c1cb4c6aebebe4809f

comment:6 Changed 7 years ago by zzz

So at Rate.java line 440:

            final double avg = useLifetime ? getLifetimeAverageValue() : getAverageValue();
            out.setAverage(avg);

can be rewritten as:

            if (useLifetime)
                out.setAverage(getLifetimeAverageValue());

right?

comment:7 Changed 7 years ago by Zlatin Balevsky

Yes

Note: See TracTickets for help on using tickets.