Hey Jun,
I think the Closable interface is what we've used elsewhere and what the
rest of the java world uses. I don't think it is too hard for us to add the
override in our interface--implementors of the interface don't need to do
it.
-Jay
On Thu, Apr 30, 2015 at 4:02 PM, Jun Rao <j...@confluent.io> wrote:
> That makes sense. Then, would it be better to have a KafkaClosable
> interface that doesn't throw exception? This way, we don't need to override
> close in every implementing class.
>
> Thanks,
>
> Jun
>
> On Wed, Apr 29, 2015 at 10:36 AM, Steven Wu <stevenz...@gmail.com> wrote:
>
> > Jun,
> >
> > we still get the benefit of extending Closeable. e.g.
> Utils.closeQuietly()
> > can take FooSerializer as an argument. we can avoid the duplication of
> > boiler-plate code.
> >
> > class FooSerializer implements Serializer {
> >
> > @Override
> > public void close() {
> > // may throw unchecked RuntimeException
> > }
> > }
> >
> > final class Utils {
> > public static void closeQuietly(Closeable c, String name,
> > AtomicReference<Throwable> firstException) {
> > if (c != null) {
> > try {
> > c.close();
> > } catch (Throwable t) {
> > firstException.compareAndSet(null, t);
> > log.error("Failed to close " + name, t);
> > }
> > }
> > }
> >
> > On Wed, Apr 29, 2015 at 6:51 AM, Jun Rao <j...@confluent.io> wrote:
> >
> > > If you do this, the code is no longer simple, which defeats the benefit
> > of
> > > extending Closeable. We can define our own Closeable that doesn't throw
> > > exceptions, but it may be confusing. So, it seems the original code is
> > > probably better.
> > >
> > > Thanks,
> > >
> > > Jun
> > >
> > > On Tue, Apr 28, 2015 at 3:11 PM, Steven Wu <stevenz...@gmail.com>
> wrote:
> > >
> > > > sorry for the previous empty msg.
> > > >
> > > > Jay's idea should work. basically, we override the close method in
> > > > Serializer interface.
> > > >
> > > > public interface Serializer<T> extends Closeable {
> > > > @Override
> > > > public void close();
> > > > }
> > > >
> > > > On Tue, Apr 28, 2015 at 1:10 PM, Steven Wu <stevenz...@gmail.com>
> > wrote:
> > > >
> > > > >
> > > > >
> > > > > On Tue, Apr 28, 2015 at 1:03 PM, Ewen Cheslack-Postava <
> > > > e...@confluent.io>
> > > > > wrote:
> > > > >
> > > > >> Good point Jay. More specifically we were already implementing
> > without
> > > > the
> > > > >> checked exception, we'd need to override close() in the Serializer
> > and
> > > > >> Deserializer interfaces and omit the throws clause. That
> definitely
> > > > makes
> > > > >> them source compatible. Not sure about binary compatibility, I
> > > couldn't
> > > > >> find a quick answer but I think it's probably still compatible.
> > > > >>
> > > > >> -Ewen
> > > > >>
> > > > >> On Tue, Apr 28, 2015 at 12:30 PM, Jay Kreps <jay.kr...@gmail.com>
> > > > wrote:
> > > > >>
> > > > >> > Hey guys,
> > > > >> >
> > > > >> > You can implement Closable without the checked exception. Having
> > > > close()
> > > > >> > methods throw checked exceptions isn't very useful unless there
> > is a
> > > > way
> > > > >> > for the caller to recover. In this case there really isn't,
> right?
> > > > >> >
> > > > >> > -Jay
> > > > >> >
> > > > >> > On Mon, Apr 27, 2015 at 5:51 PM, Guozhang Wang <
> > wangg...@gmail.com>
> > > > >> wrote:
> > > > >> >
> > > > >> > > Folks,
> > > > >> > >
> > > > >> > > In a recent commit I made regarding KAFKA-2121, there is an
> > > omitted
> > > > >> API
> > > > >> > > change which makes Serializer / Deserializer extending from
> > > > Closeable,
> > > > >> > > whose close() call could throw IOException by declaration.
> Hence
> > > now
> > > > >> some
> > > > >> > > scenario like:
> > > > >> > >
> > > > >> > > ---------------------
> > > > >> > >
> > > > >> > > Serializer<T> keySerializer = ...
> > > > >> > > Serializer<T> valueSerializer = ...
> > > > >> > > KafkaProducer producer = new KafkaProducer(config,
> > keySerializer,
> > > > >> > > valueSerializer)
> > > > >> > > // ...
> > > > >> > > keySerializer.close()
> > > > >> > > valueSerializer.close()
> > > > >> > >
> > > > >> > > ---------------------
> > > > >> > >
> > > > >> > > will need to capture IOException now.
> > > > >> > >
> > > > >> > > Want to bring this up for people's attention, and you opinion
> on
> > > > >> whether
> > > > >> > we
> > > > >> > > should revert this change?
> > > > >> > >
> > > > >> > > -- Guozhang
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >>
> > > > >>
> > > > >> --
> > > > >> Thanks,
> > > > >> Ewen
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>
