[tor-bugs] #19259 [Metrics/Onionoo]: uncaught NFE

[Tor Bug Tracker & Wiki]

#19259: uncaught NFE
-----------------------------+-----------------------------------
 Reporter:  iwakeh           |          Owner:  iwakeh
     Type:  defect           |         Status:  needs_information
 Priority:  High             |      Milestone:
Component:  Metrics/Onionoo  |        Version:
 Severity:  Major            |     Resolution:
 Keywords:                   |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:                   |        Sponsor:
-----------------------------+-----------------------------------

Comment (by iwakeh):

 Thanks for working your way through all this!
 First some responses and below some notes about the tests that might save
 you some time reviewing the test class.

 > ...
 > `WeightsStatus` is mostly an internal status class, and it seemed okay
 to use `-1.0` to encode "missing value".  The check for `< -0.5` was to
 picked to avoid any trouble with double value comparisons, like `< 0.0` or
 `== -1.0`.  Under normal circumstances, values would never have any other
 negative value than `-1.0`.  I'd say let's test this as is and then
 consider changing `weights` to `Double[]` to be able to store `null`
 values.
 >
 > The `missingValues += 1 << i` part makes sure that we only combine two
 lines with the exact same pattern of missing values.  We could have used a
 BitSet or anything else, but using bits in an `int` seemed okay here, also
 with regard to performance and avoiding the overhead of using an object
 there.  Documentation would not have hurt, of course. ;)  I'm not saying
 that this is perfect, but at least I believe the code does what it's
 intended to do.

 I'm not concerned about the bit approach, that's fine. The condition
 should be tested; indirectly this is suggested in `testMissingValues`, so
 putting it into a separate method might not be necessary.

 Regarding the symbol for missing values:
 [https://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#NaN
 `Double.Nan`] is a `double`, so it could be used here; but of course this
 needs some adaptions in the methods that convert to doc string.


 > This seems correct.  I can't say whether a rejection should be logged,
 because sometimes it might be useful to simply attempt to add an interval
 and not have the code yell if that is not possible.
 Maybe logging it in debug level will help with future troubleshooting?

 >
 > > * History intervals are supposed to be a minimum size, which?
 >
 > Huh, why?
 >
 I'll get back to this question later.

 > Errrr, can you give an example for when this could go wrong? :)  Or was
 that a suggestion for rewriting that code to make it "even" more readable?
 >
 The latter :-) but I also wanted to make sure that it describes what was
 intended once upon the time.


 > `WeightsStatus` "shares some code" with other classes in the same
 package.  The reason is that it was easier to take an existing class and
 change it as needed than generalizing and writing subclasses.  I'd say
 let's clean up `WeightsStatus` first and then see what changes we can
 adapt in other classes.  In theory, by simplifying `WeightsStatus`, we'll
 get closer to a state where we can generalize classes and put common parts
 in a superclass.  But let's not aim for that just yet, if that's okay for
 you?

 I think, we should limit this for the first release to verifying other
 comparator implementations and singling out unused methods.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19259#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online