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

[Yishun Guan]

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, 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 extend AutoClosable, so method signature won't be changed. Hence it
> should be fine to backward compatibility.
>
> On 2018/09/30 20:19:57, "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.
> >
> > Thus, I am not sure atm which one suits better.
> >
> > 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.
> >
> > Nit: `RecordCollectorImpl` is an internal class that implements
> > `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
> >
> >
> > -Matthias
> >
> >
> > On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
> > >> (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 exception...
> > >
> > > On 2018/09/27 19:11:20, Yishun Guan  wrote:
> > >> 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.
> > >>
> > >> Thanks,
> > >> Yishun
> > >> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee 
> wrote:
> > >>>
> > >>> 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:
> > >>>
> >  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) ExpiringCredentialRefreshingLogin
> >  5) KafkaChannel
> >  6) ConsumerInterceptor
> >  7) SelectorMetrics
> >  8) HeartbeatThread
> > 
> >  Cheers,
> >  Chia-Ping
> > 
> > 
> >  On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > > Hi All,
> > >
> > > Here is a trivial KIP:
> > >
> > 
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> > >
> > > Suggestions are welcome.
> > >
> > > Thanks,
> > > Yishun
> > >
> > 
> > >>>
> > >>>
> > >>> --
> > >>> *Dongjin Lee*
> > >>>
> > >>> *A hitchhiker in the mathematical world.*
> > >>>
> > >>> *github:  github.com/dongjinleekr
> > >>> linkedin:
> kr.linkedin.com/in/dongjinleekr
> > >>> slideshare:
> > >>> www.slideshare.net/dongjinleekr
> > >>> *
> > >>
> >
> >
>

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

[Chia-Ping Tsai]

> 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 
extend AutoClosable, so method signature won't be changed. Hence it should be 
fine to backward compatibility.

On 2018/09/30 20:19:57, "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.
> 
> Thus, I am not sure atm which one suits better.
> 
> 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.
> 
> Nit: `RecordCollectorImpl` is an internal class that implements
> `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
> 
> 
> -Matthias
> 
> 
> On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
> >> (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 exception...
> > 
> > On 2018/09/27 19:11:20, Yishun Guan  wrote: 
> >> 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.
> >>
> >> Thanks,
> >> Yishun
> >> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
> >>>
> >>> 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:
> >>>
>  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) ExpiringCredentialRefreshingLogin
>  5) KafkaChannel
>  6) ConsumerInterceptor
>  7) SelectorMetrics
>  8) HeartbeatThread
> 
>  Cheers,
>  Chia-Ping
> 
> 
>  On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > Hi All,
> >
> > Here is a trivial KIP:
> >
>  https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> >
> > Suggestions are welcome.
> >
> > Thanks,
> > Yishun
> >
> 
> >>>
> >>>
> >>> --
> >>> *Dongjin Lee*
> >>>
> >>> *A hitchhiker in the mathematical world.*
> >>>
> >>> *github:  github.com/dongjinleekr
> >>> linkedin: kr.linkedin.com/in/dongjinleekr
> >>> slideshare:
> >>> www.slideshare.net/dongjinleekr
> >>> *
> >>
> 
> 

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

[John Roesler]

> 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 invocations, but there's the off-chance
that someone has assigned an instance to a Closeable variable, which would
break.

Overalll, it seems like maybe we should leave any Closeable implementations
alone and just add AutoCloseable where there's no existing closeable
interface.

-John

On Wed, Oct 3, 2018 at 12:06 PM Matthias J. Sax 
wrote:

> 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 motivation of this KIP to begin with. Also note, that `Closable
> extends AutoClosable`. Thus, both interfaces work with try-with-resource.
>
> Overall, it seems that `AutoClosable` might be the better interface to
> use though because it's more generic.
>
> -Matthias
>
>
> On 10/3/18 9:48 AM, Colin McCabe wrote:
> > 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.
> >
> > That's an interesting note.   Looks like the exact JavaDoc text is:
> >
> >  > Note that unlike the close method of Closeable, this close method is
> not
> >  > required to be idempotent. In other words, calling this close method
> more
> >  > than once may have some visible side effect, unlike Closeable.close
> which
> >  > is required to have no effect if called more than once. However,
> >  > implementers of this interface are strongly encouraged to make their
> close
> >  > methods idempotent.
> >
> > So you can make it non-idempotent, but it's still recommended to make it
> idempotent.
> >
> >>
> >> Thus, I am not sure atm which one suits better.
> >>
> >> 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.
> >
> > I was worried about that too, but actually you can implement the
> AutoCloseable interface without declaring "throws Exception".  In general,
> you can implement an interface while throwing a subset of the possible
> checked exceptions.
> >
> > There is one big benefit of AutoCloseable that I haven't seen mentioned
> here yet: the ability to use constructrs like try-with-resources
> transparently.  So you can do things like
> >
> >> try (MyClass m = new MyClass()) {
> >>   m.doSomething(...);
> >> }
> >
> > best,
> > Colin
> >
> >>
> >> Nit: `RecordCollectorImpl` is an internal class that implements
> >> `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
> >>
> >>
> >> -Matthias
> >>
> >>
> >> On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
>  (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 exception...
> >>>
> >>> On 2018/09/27 19:11:20, Yishun Guan  wrote:
>  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.
> 
>  Thanks,
>  Yishun
>  On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee 
> wrote:
> >
> > 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:
> >
> >> 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) ExpiringCredentialRefreshingLogin
> >> 5) KafkaChannel
> >> 6) ConsumerInterceptor
> >> 7) 

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

[Matthias J. Sax]

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 motivation of this KIP to begin with. Also note, that `Closable
extends AutoClosable`. Thus, both interfaces work with try-with-resource.

Overall, it seems that `AutoClosable` might be the better interface to
use though because it's more generic.

-Matthias


On 10/3/18 9:48 AM, Colin McCabe wrote:
> 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.
> 
> That's an interesting note.   Looks like the exact JavaDoc text is:
> 
>  > Note that unlike the close method of Closeable, this close method is not 
>  > required to be idempotent. In other words, calling this close method more 
>  > than once may have some visible side effect, unlike Closeable.close which 
>  > is required to have no effect if called more than once. However, 
>  > implementers of this interface are strongly encouraged to make their close 
>  > methods idempotent.
> 
> So you can make it non-idempotent, but it's still recommended to make it 
> idempotent.
> 
>>
>> Thus, I am not sure atm which one suits better.
>>
>> 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.
> 
> I was worried about that too, but actually you can implement the 
> AutoCloseable interface without declaring "throws Exception".  In general, 
> you can implement an interface while throwing a subset of the possible 
> checked exceptions.
> 
> There is one big benefit of AutoCloseable that I haven't seen mentioned here 
> yet: the ability to use constructrs like try-with-resources transparently.  
> So you can do things like
> 
>> try (MyClass m = new MyClass()) {
>>   m.doSomething(...);
>> }
> 
> best,
> Colin
> 
>>
>> Nit: `RecordCollectorImpl` is an internal class that implements
>> `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
>>
>>
>> -Matthias
>>
>>
>> On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
 (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 exception...
>>>
>>> On 2018/09/27 19:11:20, Yishun Guan  wrote: 
 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.

 Thanks,
 Yishun
 On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
>
> 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:
>
>> 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) ExpiringCredentialRefreshingLogin
>> 5) KafkaChannel
>> 6) ConsumerInterceptor
>> 7) SelectorMetrics
>> 8) HeartbeatThread
>>
>> Cheers,
>> Chia-Ping
>>
>>
>> On 2018/09/26 23:44:31, Yishun Guan  wrote:
>>> Hi All,
>>>
>>> Here is a trivial KIP:
>>>
>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
>>>
>>> Suggestions are welcome.
>>>
>>> Thanks,
>>> Yishun
>>>
>>
>
>
> --
> *Dongjin Lee*
>
> *A hitchhiker in the mathematical world.*
>
> *github:  github.com/dongjinleekr
> linkedin: kr.linkedin.com/in/dongjinleekr
> slideshare:
> www.slideshare.net/dongjinleekr
> *

>>
>> Email had 1 attachment:
>> + signature.asc

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

[Yishun Guan]

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.

Thanks!
Yishun

On Wed, Oct 3, 2018, 9:48 AM Colin McCabe  wrote:

> 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.
>
> That's an interesting note.   Looks like the exact JavaDoc text is:
>
>  > Note that unlike the close method of Closeable, this close method is
> not
>  > required to be idempotent. In other words, calling this close method
> more
>  > than once may have some visible side effect, unlike Closeable.close
> which
>  > is required to have no effect if called more than once. However,
>  > implementers of this interface are strongly encouraged to make their
> close
>  > methods idempotent.
>
> So you can make it non-idempotent, but it's still recommended to make it
> idempotent.
>
> >
> > Thus, I am not sure atm which one suits better.
> >
> > 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.
>
> I was worried about that too, but actually you can implement the
> AutoCloseable interface without declaring "throws Exception".  In general,
> you can implement an interface while throwing a subset of the possible
> checked exceptions.
>
> There is one big benefit of AutoCloseable that I haven't seen mentioned
> here yet: the ability to use constructrs like try-with-resources
> transparently.  So you can do things like
>
> > try (MyClass m = new MyClass()) {
> >   m.doSomething(...);
> > }
>
> best,
> Colin
>
> >
> > Nit: `RecordCollectorImpl` is an internal class that implements
> > `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
> >
> >
> > -Matthias
> >
> >
> > On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
> > >> (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 exception...
> > >
> > > On 2018/09/27 19:11:20, Yishun Guan  wrote:
> > >> 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.
> > >>
> > >> Thanks,
> > >> Yishun
> > >> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee 
> wrote:
> > >>>
> > >>> 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:
> > >>>
> >  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) ExpiringCredentialRefreshingLogin
> >  5) KafkaChannel
> >  6) ConsumerInterceptor
> >  7) SelectorMetrics
> >  8) HeartbeatThread
> > 
> >  Cheers,
> >  Chia-Ping
> > 
> > 
> >  On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > > Hi All,
> > >
> > > Here is a trivial KIP:
> > >
> > 
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> > >
> > > Suggestions are welcome.
> > >
> > > Thanks,
> > > Yishun
> > >
> > 
> > >>>
> > >>>
> > >>> --
> > >>> *Dongjin Lee*
> > >>>
> > >>> *A hitchhiker in the mathematical world.*
> > >>>
> > >>> *github:  github.com/dongjinleekr
> > >>> linkedin:
> kr.linkedin.com/in/dongjinleekr
> > >>> slideshare:
> > >>> www.slideshare.net/dongjinleekr
> > >>> *
> > >>
> >
> > Email had 1 attachment:
> > + signature.asc
> >   1k (application/pgp-signature)
>

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

[Colin McCabe]

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.

That's an interesting note.   Looks like the exact JavaDoc text is:

 > Note that unlike the close method of Closeable, this close method is not 
 > required to be idempotent. In other words, calling this close method more 
 > than once may have some visible side effect, unlike Closeable.close which 
 > is required to have no effect if called more than once. However, 
 > implementers of this interface are strongly encouraged to make their close 
 > methods idempotent.

So you can make it non-idempotent, but it's still recommended to make it 
idempotent.

> 
> Thus, I am not sure atm which one suits better.
> 
> 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.

I was worried about that too, but actually you can implement the AutoCloseable 
interface without declaring "throws Exception".  In general, you can implement 
an interface while throwing a subset of the possible checked exceptions.

There is one big benefit of AutoCloseable that I haven't seen mentioned here 
yet: the ability to use constructrs like try-with-resources transparently.  So 
you can do things like

> try (MyClass m = new MyClass()) {
>   m.doSomething(...);
> }

best,
Colin

> 
> Nit: `RecordCollectorImpl` is an internal class that implements
> `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
> 
> 
> -Matthias
> 
> 
> On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
> >> (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 exception...
> > 
> > On 2018/09/27 19:11:20, Yishun Guan  wrote: 
> >> 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.
> >>
> >> Thanks,
> >> Yishun
> >> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
> >>>
> >>> 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:
> >>>
>  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) ExpiringCredentialRefreshingLogin
>  5) KafkaChannel
>  6) ConsumerInterceptor
>  7) SelectorMetrics
>  8) HeartbeatThread
> 
>  Cheers,
>  Chia-Ping
> 
> 
>  On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > Hi All,
> >
> > Here is a trivial KIP:
> >
>  https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> >
> > Suggestions are welcome.
> >
> > Thanks,
> > Yishun
> >
> 
> >>>
> >>>
> >>> --
> >>> *Dongjin Lee*
> >>>
> >>> *A hitchhiker in the mathematical world.*
> >>>
> >>> *github:  github.com/dongjinleekr
> >>> linkedin: kr.linkedin.com/in/dongjinleekr
> >>> slideshare:
> >>> www.slideshare.net/dongjinleekr
> >>> *
> >>
> 
> Email had 1 attachment:
> + signature.asc
>   1k (application/pgp-signature)

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

[Yishun Guan]

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 'close()' doesn't have to throw an
exception (either throw a subclass of the parent exception, or not
throw one at all is fine, I could be totally wrong.)

Chia-Ping, I am not quite sure I understand your idea. With the same
idea above, `AutoClosable#close()` is board enough to cover unchecked
and checked exception, right? Could you elaborate?

Thanks,
Yishun
On Sun, Sep 30, 2018 at 1:20 PM 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.
>
> Thus, I am not sure atm which one suits better.
>
> 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.
>
> Nit: `RecordCollectorImpl` is an internal class that implements
> `RecordCollector` -- should `RecordCollector extends AutoCloseable`?
>
>
> -Matthias
>
>
> On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
> >> (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 exception...
> >
> > On 2018/09/27 19:11:20, Yishun Guan  wrote:
> >> 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.
> >>
> >> Thanks,
> >> Yishun
> >> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
> >>>
> >>> 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:
> >>>
>  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) ExpiringCredentialRefreshingLogin
>  5) KafkaChannel
>  6) ConsumerInterceptor
>  7) SelectorMetrics
>  8) HeartbeatThread
> 
>  Cheers,
>  Chia-Ping
> 
> 
>  On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > Hi All,
> >
> > Here is a trivial KIP:
> >
>  https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> >
> > Suggestions are welcome.
> >
> > Thanks,
> > Yishun
> >
> 
> >>>
> >>>
> >>> --
> >>> *Dongjin Lee*
> >>>
> >>> *A hitchhiker in the mathematical world.*
> >>>
> >>> *github:  github.com/dongjinleekr
> >>> linkedin: kr.linkedin.com/in/dongjinleekr
> >>> slideshare:
> >>> www.slideshare.net/dongjinleekr
> >>> *
> >>
>

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

[Matthias J. Sax]

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 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.

Nit: `RecordCollectorImpl` is an internal class that implements
`RecordCollector` -- should `RecordCollector extends AutoCloseable`?


-Matthias


On 9/27/18 7:46 PM, Chia-Ping Tsai wrote:
>> (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 exception...
> 
> On 2018/09/27 19:11:20, Yishun Guan  wrote: 
>> 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.
>>
>> Thanks,
>> Yishun
>> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
>>>
>>> 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:
>>>
 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) ExpiringCredentialRefreshingLogin
 5) KafkaChannel
 6) ConsumerInterceptor
 7) SelectorMetrics
 8) HeartbeatThread

 Cheers,
 Chia-Ping


 On 2018/09/26 23:44:31, Yishun Guan  wrote:
> Hi All,
>
> Here is a trivial KIP:
>
 https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
>
> Suggestions are welcome.
>
> Thanks,
> Yishun
>

>>>
>>>
>>> --
>>> *Dongjin Lee*
>>>
>>> *A hitchhiker in the mathematical world.*
>>>
>>> *github:  github.com/dongjinleekr
>>> linkedin: kr.linkedin.com/in/dongjinleekr
>>> slideshare:
>>> www.slideshare.net/dongjinleekr
>>> *
>>



signature.asc
Description: OpenPGP digital signature

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

[Chia-Ping Tsai]

> (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 exception...

On 2018/09/27 19:11:20, Yishun Guan  wrote: 
> 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.
> 
> Thanks,
> Yishun
> On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
> >
> > 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:
> >
> > > 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) ExpiringCredentialRefreshingLogin
> > > 5) KafkaChannel
> > > 6) ConsumerInterceptor
> > > 7) SelectorMetrics
> > > 8) HeartbeatThread
> > >
> > > Cheers,
> > > Chia-Ping
> > >
> > >
> > > On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > > > Hi All,
> > > >
> > > > Here is a trivial KIP:
> > > >
> > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> > > >
> > > > Suggestions are welcome.
> > > >
> > > > Thanks,
> > > > Yishun
> > > >
> > >
> >
> >
> > --
> > *Dongjin Lee*
> >
> > *A hitchhiker in the mathematical world.*
> >
> > *github:  github.com/dongjinleekr
> > linkedin: kr.linkedin.com/in/dongjinleekr
> > slideshare:
> > www.slideshare.net/dongjinleekr
> > *
> 

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

[Yishun Guan]

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.

Thanks,
Yishun
On Thu, Sep 27, 2018 at 3:26 AM Dongjin Lee  wrote:
>
> 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:
>
> > 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) ExpiringCredentialRefreshingLogin
> > 5) KafkaChannel
> > 6) ConsumerInterceptor
> > 7) SelectorMetrics
> > 8) HeartbeatThread
> >
> > Cheers,
> > Chia-Ping
> >
> >
> > On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > > Hi All,
> > >
> > > Here is a trivial KIP:
> > >
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> > >
> > > Suggestions are welcome.
> > >
> > > Thanks,
> > > Yishun
> > >
> >
>
>
> --
> *Dongjin Lee*
>
> *A hitchhiker in the mathematical world.*
>
> *github:  github.com/dongjinleekr
> linkedin: kr.linkedin.com/in/dongjinleekr
> slideshare:
> www.slideshare.net/dongjinleekr
> *

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

[Dongjin Lee]

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:

> 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) ExpiringCredentialRefreshingLogin
> 5) KafkaChannel
> 6) ConsumerInterceptor
> 7) SelectorMetrics
> 8) HeartbeatThread
>
> Cheers,
> Chia-Ping
>
>
> On 2018/09/26 23:44:31, Yishun Guan  wrote:
> > Hi All,
> >
> > Here is a trivial KIP:
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> >
> > Suggestions are welcome.
> >
> > Thanks,
> > Yishun
> >
>


-- 
*Dongjin Lee*

*A hitchhiker in the mathematical world.*

*github:  github.com/dongjinleekr
linkedin: kr.linkedin.com/in/dongjinleekr
slideshare:
www.slideshare.net/dongjinleekr
*

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

[Chia-Ping Tsai]

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) ExpiringCredentialRefreshingLogin
5) KafkaChannel
6) ConsumerInterceptor
7) SelectorMetrics
8) HeartbeatThread

Cheers,
Chia-Ping


On 2018/09/26 23:44:31, Yishun Guan  wrote: 
> Hi All,
> 
> Here is a trivial KIP:
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> 
> Suggestions are welcome.
> 
> Thanks,
> Yishun
> 

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

[Yishun Guan]

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 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 be explicit with the
> proposed changes in a KIP
>
> Also, you are proposing changes across several components and I'm not sure
> if that is possible in a single KIP, but I could very well be wrong on this
> one, so we'll see what others say.
>
> Thanks,
> Bill
>
> On Wed, Sep 26, 2018 at 7:44 PM Yishun Guan  wrote:
>
> > Hi All,
> >
> > Here is a trivial KIP:
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=93325308
> >
> > Suggestions are welcome.
> >
> > Thanks,
> > Yishun
> >

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

[Bill Bejeck]

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 be explicit with the
proposed changes in a KIP

Also, you are proposing changes across several components and I'm not sure
if that is possible in a single KIP, but I could very well be wrong on this
one, so we'll see what others say.

Thanks,
Bill

On Wed, Sep 26, 2018 at 7:44 PM Yishun Guan  wrote:

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