addthis / stream-lib

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

CountMinSketch is not serializable #109

Closed gokyo closed 8 years ago

gokyo commented 8 years ago

Could we make com.clearspring.analytics.stream.frequency.CountMinSketch implementing java.io.Serializable please?

I am getting this error: org.apache.commons.lang3.SerializationException: java.io.NotSerializableException: com.clearspring.analytics.stream.frequency.CountMinSketch at org.apache.commons.lang3.SerializationUtils.serialize(SerializationUtils.java:156)

abramsm commented 8 years ago

CountMinSketch has serialize and deserialize in the current implementation. It should be possible to extend the Serializable interface as well. Have you looked to see if using the serialize/deserialize methods is viable for you? Are you interested in submitting a pull request that makes the desired modification?

gokyo commented 8 years ago

Using serialize and deserialize would make my code complex. Best for me would be to implement Serializable as it requires only a simple change on your code, and no changes on my code. I can create a pull request if it is fine for you guys. Thanks

gokyo commented 8 years ago

I created the pull request on here: https://github.com/addthis/stream-lib/pull/110 On my local machine the Maven build passed. Both mvn clean install test and mvn test -B build successfully. It is compatible with both Java 7 and Java 8. But one of your three builds failed unfortunately: https://travis-ci.org/addthis/stream-lib/builds/136229647 I believe my code changes are fine. Let me know if the change can be merged and released. Thanks

gokyo commented 8 years ago

Hi Any news on this ticket? Thanks

yuesong commented 8 years ago

Sorry for the delay. The code looks fine. I'll do a final pass over.

gokyo commented 8 years ago

Thanks Let me know when can be merged and released :+1:

gokyo commented 8 years ago

I close the issue as the code has been merged

yuesong commented 8 years ago

It's released.

gokyo commented 8 years ago

Thanks :+1: