Remove IOException from ICardinality.getBytes() #34
Conversation
By using a ByteBuffer instead of a ByteArrayOutputStream we can get ride of cumbersome IOException. We also get an extra boost for free. Running TestHyperLogLog.testSerialization in a caliper micro-benchmark shows that the ByteBuffer version is two times faster.
By using a ByteBuffer instead of a ByteArrayOutputStream we can get ride of cumbersome IOException. We also get an extra boost for free.
We can safely try/catch exceptions in ExternalizableUtil since ByteArrayOutputStream never throws IOException.
All implementations have been cleaned. Their is no reason for a serializer to throw IOException and users don't want to criple their code with unecessary exception handling.
|
working my way through testing this one. thanks for the patch. |
|
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. |
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:
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.