Skip to content
This repository has been archived by the owner on Jul 7, 2020. It is now read-only.

CountMinSketch is not serializable #109

Closed
gokyo opened this issue Jun 8, 2016 · 9 comments
Closed

CountMinSketch is not serializable #109

gokyo opened this issue Jun 8, 2016 · 9 comments

Comments

@gokyo
Copy link

gokyo commented Jun 8, 2016

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
Copy link
Contributor

abramsm commented Jun 8, 2016

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
Copy link
Author

gokyo commented Jun 8, 2016

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
Copy link
Author

gokyo commented Jun 8, 2016

I created the pull request on here: #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
Copy link
Author

gokyo commented Jun 15, 2016

Hi
Any news on this ticket?
Thanks

@yuesong
Copy link
Contributor

yuesong commented Jun 15, 2016

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

@gokyo
Copy link
Author

gokyo commented Jun 15, 2016

Thanks
Let me know when can be merged and released 👍

@gokyo
Copy link
Author

gokyo commented Jun 15, 2016

I close the issue as the code has been merged

@gokyo gokyo closed this as completed Jun 15, 2016
@yuesong
Copy link
Contributor

yuesong commented Jun 15, 2016

It's released.

@gokyo
Copy link
Author

gokyo commented Jun 15, 2016

Thanks 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants