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