Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi All, I started a vote on this KIP on another email thread, but it didn't generate responses. So I decided to start a vote on this thread. Thanks! Link: https://www.zhihu.com/question/21491539 Best Yishun On Thu, Oct 4, 2018 at 11:37 PM Chia-Ping Tsai wrote: > > However, it's a good hint,

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

> However, it's a good hint, that `AutoClosable#close()` declares `throws > Exception` and thus, it seems to be a backward incompatible change. > Hence, I am not sure if we can actually move forward easily with this KIP. It is legal to remove declaration of "throwing Exception" from a class which

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

> Overall, it seems that `AutoClosable` might be the better interface to > use though because it's more generic. This sounds good to me. I don't know whether or not it's worth actually transitioning any existing classes from Closeable to AutoCloseable. I don't think it would affect any

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Thanks for clarifying. I thought that if we inherit `close() throws Exception` we need to declare the same exception -- this would have been an issue. Thus, my backward compatibility concerns are resolved. About try-with-resources: I think, allowing to use try-with-resources is the core

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi Colin, Yes, I absolutely agree with you. We can implement an interface while throwing a subset of the possible checked exceptions, and empty set is also a subset-so we also don't have to throw an exception. And yes, you are right, I should emphasize the benefit of AutoCloseable in the KIP.

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

On Sun, Sep 30, 2018, at 13:19, Matthias J. Sax wrote: > Closeable is part of `java.io` while AutoClosable is part of > `java.lang`. Thus, the second one is more generic. Also, JavaDoc points > out that `Closable#close()` must be idempotent while > `AutoClosable#close()` can have side effects.

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi Matthias, thank you for pointing this out! I have changed the KIP to 'RecordCollector extends AutoCloseable`. As your first concern regarding incompatibility, can you explain why this is a breaking change? Although that `AutoClosable#close()` declares `throws Exception, I think the overridden

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Closeable is part of `java.io` while AutoClosable is part of `java.lang`. Thus, the second one is more generic. Also, JavaDoc points out that `Closable#close()` must be idempotent while `AutoClosable#close()` can have side effects. Thus, I am not sure atm which one suits better. However, it's a

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

> (Although I am not quite sure > when one is more desirable than the other) Most kafka's classes implementing Closeable/AutoCloseable doesn't throw checked exception in close() method. Perhaps we should have a "KafkaCloseable" interface which has a close() method without throwing any checked

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi All, Chia-Ping, I agree, similar to VarifiableConsumer, VarifiableProducer should be implementing Closeable as well (Although I am not quite sure when one is more desirable than the other), also I just looked through your list - these are some great additions, I will add them to the list.

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi Yishun, Thank you for your great KIP. In fact, I have also encountered the cases where Autoclosable is so desired several times! Let me inspect more candidate classes as well. +1. I also refined your KIP a little bit. Best, Dongjin On Thu, Sep 27, 2018 at 12:21 PM Chia-Ping Tsai wrote: >

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

hi Yishun Thanks for nice KIP! Q1) Why VerifiableProducer extend Closeable rather than AutoCloseable? Q2) I grep project and then noticed there are other close methods but do not implement AutoCloseable. For example: 1) WorkerConnector 2) MemoryRecordsBuilder 3) MetricsReporter 4)

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi Bill, I see, i just updated the KIP with all the changes. And your second concern make sense - if that's what everyone think, we can start with changing KafkaStream first. Thanks, Yishun On Wed, Sep 26, 2018 at 5:18 PM Bill Bejeck wrote: > > Hi Yishun, > > Thanks for the KIP, seems like a

Re: [DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi Yishun, Thanks for the KIP, seems like a useful addition. Just a couple of minor comments. Can you list all the changes in the list under "Public Interfaces" in the "Proposed Changes" section that way it's clear what's changing? I realize they all will be very similar, but it's better to

[DISCUSSION] KIP-376: Implement AutoClosable on appropriate classes that has close()

Hi All, Here is a trivial KIP: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308 Suggestions are welcome. Thanks, Yishun