addthis
Remove IOException from ICardinality.getBytes()
I extensively use the built-in serialization/de-serialization mechanism offered by ICardinality estimators and was a bit irritated by having to catch cumbersome and useless IOExceptions. As long as we are working with byte buffers their is no reason to throw IOException to the caller.
This pull request update the ICardinality.getBytes() method to remove IOException:
- HyperLogLog and HyperLogLogPlus have been update to use ByteBuffer rather than ByteArrayOutputStream. No more IOExceptions and it is faster
- ExternalizableUtil.toBytes() catchs IOExceptions and rethrow an unchecked IllegalStateException. IOException will never be thrown since we are using a ByteArrayOutputStream.
- Builders have also been update to remove IOException
I took care to not change the serialization format to be backward compatible. API changes should also be backward compatible for client. The only possible issue would be a third party implementation of ICardinality throwing an IOException.
What I said was wrong. This one breaks source compatibility since it removes a checked exception from method signature.
I did a hprof session on one of our batch today with all my patches. The serialization/de-serialization speed-up is noticeable. If source compatibility is important, we can split this patch set in two.. We can replace the underlying implementation now and wait the next API breakage to remove the IOException.
I'm OK with API breaking changes in this release if its the right thing to do.
I will have another API change in ICardinality to submit in few days (regarding the merge method). Let's see if you agree with the other one, and then decide what to do for this one. I don't think it worth breaking the source compatibility only for one IOException.
If you decide to reject the other API change, we can still split the patch set.
OK, thanks I'll wait till you are ready with the other patch before merging this one. Code looks good though and passes tests.