search

addthis / stream-lib

Stream summarizer and cardinality estimator.
Apache License 2.0
2.26k stars 556 forks source link

Made HyperLogLog serializable. #70

Closed rxin closed 10 years ago

rxin commented 10 years ago

See the discussion here: https://groups.google.com/forum/#!topic/stream-lib-user/jLDVn27ZYPE

Happy to make changes as requested. The no-arg constructor is a little bit ugly.

rxin commented 10 years ago

I'm new to this, but Travis' failure seems to be on a different code path than the ones I changed?

tea-dragon commented 10 years ago

I couldn't reproduce the error (or even anything close to it). It is also strange that this only happened on openjdk6. Admittedly, I don't have that jdk on hand, but it ran fine locally under oraclejdk6 and openjdk7.

I did some math, and I probably made a mistake somewhere, but I think that test is actually only expected to pass 99.73% of the time (although in this case there was only a 0.05% chance for it to do as badly as it did). I suppose with enough automatic travis testing that would occur sooner or later. It seems to do three test runs every time so I suppose that adds up.

In any event, I think we can all agree it was unlikely to be related to your changes. I ran the math because if it had turned out to be one in a million or something then I would have felt compelled to investigate the possibility of a rare (but realistically rare -- similar to the 99.73% result) bug that causes large accuracy loss.

tea-dragon commented 10 years ago

Please take a look at https://github.com/addthis/stream-lib/pull/71 and let me know if you feel okay about closing this one.

rxin commented 10 years ago

Closing in favor of #71.