apache / iceberg

Apache Iceberg

https://iceberg.apache.org/

Apache License 2.0

6.49k stars 2.24k forks source link

Kafka Connect: Fix a bug in streams closing while read or write metadata files #11609

Open hussein-awala opened 1 day ago

hussein-awala commented 1 day ago

closes: #11582

11220 introduced a new bug instead of fixing the original issue, where it fixed the case where there is an exception in the `GZIPOutputStream` constructor, but if there is not, the stream will be closed when try-with-resources tries to close it and it will throw an exception.

This PR:

reverts the change
adds a comment to avoid breaking it in the future
and closes the stream in a try/except block to fix the main issue that #11220 tried to fix

bryanck commented 12 hours ago

+1 to reverting the original change

hussein-awala commented 12 hours ago

+1 to reverting the original change

Yes, but there is indeed an edge case that leads to failure, and this fix is a revert + fix for the original problem. Manually closing the file stream that can be closed automatically is ugly, I agree, but sometimes we have to do it.

I'm open to any suggestion

bryanck commented 12 hours ago

For 1.7.1 I'd prefer to revert it, and follow up with a separate PR with a fix, but I'm OK either way.

hussein-awala commented 11 hours ago

For 1.7.1 I'd prefer to revert it, and follow up with a separate PR with a fix

Sounds good, I opened https://github.com/apache/iceberg/pull/11621 to revert the commit.

apache / iceberg

Kafka Connect: Fix a bug in streams closing while read or write metadata files #11609

11220 introduced a new bug instead of fixing the original issue, where it fixed the case where there is an exception in the GZIPOutputStream constructor, but if there is not, the stream will be closed when try-with-resources tries to close it and it will throw an exception.

11220 introduced a new bug instead of fixing the original issue, where it fixed the case where there is an exception in the `GZIPOutputStream` constructor, but if there is not, the stream will be closed when try-with-resources tries to close it and it will throw an exception.