i agree. propagating up is the best. i was looking at NIOServerCnxn rather
than checking above. behavior will change slightly since failing on a PING
will cause another attempt at sending a response. i'm not sure how much
that will change thing, but it is a code path that we have never hit with
NIOServerCnxn.

ben

On Thu, Sep 1, 2016 at 1:17 PM, yuliya Feldman <yufeld...@yahoo.com.invalid>
wrote:

> Yes,
> This is what I plan - propagate IOException up. We could convert other
> exceptions to IOException as well and propagate up.
>
> Thank you guys for replies.Yuliya
>
>       From: Michael Han <h...@cloudera.com>
>  To: UserZooKeeper <u...@zookeeper.apache.org>
> Cc: dev@zookeeper.apache.org; yuliya Feldman <yufeld...@yahoo.com>;
> Patrick Hunt <ph...@apache.org>
>  Sent: Thursday, September 1, 2016 12:17 PM
>  Subject: Re: Issue with NettyServerCnxn.java
>
> Make sense to me.
>
> On Thu, Sep 1, 2016 at 12:10 PM, Flavio Junqueira <f...@apache.org> wrote:
>
> > I guess that's precisely what I'm proposing we avoid, I think we should
> > propagate up as an IOException, which the signature of the abstract
> method
> > already suggests we should be doing. If what I'm saying makes any sense,
> we
> > should instead remove the catch Exception block at the end of
> > NIOServerCnx.sendResponse.
> >
> > -Flavio
> >
> > > On 01 Sep 2016, at 20:05, Michael Han <h...@cloudera.com> wrote:
> > >
> > > I think it is not just about IOException - the current
> > > NIOServerCnxn.sendResponse swallows any exception it caught (including
> > the
> > > NPE this thread the related JIRA is talking about.). On the other hand,
> > the
> > > NettyServerCnx.sendResponse only catches IOException, so there is a
> > > discrepancy in terms of the behaviors of catching exception. Probably
> > > making NettyServerCnx.sendResponse catches every exception is the
> > solution
> > > here?
> > >
> > > On Thu, Sep 1, 2016 at 11:57 AM, Flavio Junqueira <f...@apache.org>
> > wrote:
> > >
> > >> I'm not sure why you say that it is better to swallow the exception,
> > Ben.
> > >> I checked the methods that call sendResponse and they seem to be able
> to
> > >> handle IOExceptions fine. For example, in NettyServerCnxn.process, we
> > call
> > >> close upon IOException, which is exactly the behavior you mention you
> > >> should have.
> > >>
> > >> I'm thinking that in this case, if the channel is closed and it is
> null,
> > >> we throw IOException. I'm trying to understand why that's bad course
> of
> > >> action.
> > >>
> > >> -Flavio
> > >>
> > >>> On 01 Sep 2016, at 19:29, Benjamin Reed <br...@apache.org> wrote:
> > >>>
> > >>> i agree, the exception should not bubble up. if something bad happens
> > we
> > >>> should mark the connection as closed (if not already) and continue
> on.
> > >>> elsewhere closed connections are cleaned up. (or at least they better
> > >> be...)
> > >>>
> > >>> ben
> > >>>
> > >>> On Thu, Sep 1, 2016 at 11:02 AM, yuliya Feldman
> > >> <yufeld...@yahoo.com.invalid
> > >>>> wrote:
> > >>>
> > >>>> Thank you Ben and Patrick for the replies.
> > >>>> The problem I see with Netty exception handling (or rather not
> > handling)
> > >>>> is that if something happens it bubbles up and main request
> processing
> > >>>> thread is stopped which effectively halts whole ZK server
> operations.
> > >>>> I will submit a JIRA on this (hopefully today). Either we should not
> > >>>> bubble up any exception by IOException or ZK server should be
> stopped,
> > >> as
> > >>>> it is really hard to figure out without turning on tracing what
> really
> > >>>> happened.
> > >>>> ThanksYuliya
> > >>>>
> > >>>>    From: Benjamin Reed <br...@apache.org>
> > >>>> To: Patrick Hunt <ph...@apache.org>
> > >>>> Cc: DevZooKeeper <dev@zookeeper.apache.org>; yuliya Feldman <
> > >>>> yufeld...@yahoo.com>; "u...@zookeeper.apache.org" <
> > >>>> u...@zookeeper.apache.org>
> > >>>> Sent: Wednesday, August 31, 2016 10:47 PM
> > >>>> Subject: Re: Issue with NettyServerCnxn.java
> > >>>>
> > >>>> if i remember correctly the case in sendResponse where it is
> catching
> > >> the
> > >>>> IOException is due to the fact that we are opportunistically trying
> to
> > >> send
> > >>>> something on a non-blocking channel. if it works, ok, but if we
> can't
> > >> send
> > >>>> because we are blocked then we will just send later.
> > >>>>
> > >>>> in the case of NIOServerCnxn there really shouldn't be any
> exceptions
> > in
> > >>>> sendResponse since it's just queuing. i think the catch is probably
> > >> there
> > >>>> so that the exception does not get propagated up and kill
> everything.
> > >>>>
> > >>>> ben
> > >>>>
> > >>>> On Wed, Aug 31, 2016 at 9:52 PM, Patrick Hunt <ph...@apache.org>
> > wrote:
> > >>>>
> > >>>>> Hi Yuliya - my read is that sendResponse in NIOServerCnxn is
> logging,
> > >>>>> then dropping, any Exceptions encountered during sendResponse. In
> > other
> > >>>>> words it's doing best effort response. Not sure if that is
> "correct",
> > >> but
> > >>>>> that's what it's currently doing in NIO. Surprisingly it's also
> > hiding
> > >>>> any
> > >>>>> IOExceptions, which is part of the method signature as defined by
> > >>>>> ServerCnxn. Some of the calling code is trying to handle
> IOException
> > in
> > >>>>> some cases which is odd... I suspect it was an oversight in
> > >>>> ZOOKEEPER-597,
> > >>>>> but I'm not sure.
> > >>>>>
> > >>>>> Ben any insight?
> > >>>>>
> > >>>>> Patrick
> > >>>>>
> > >>>>> On Tue, Aug 30, 2016 at 5:15 PM, yuliya Feldman <
> > >>>>> yufeld...@yahoo.com.invalid> wrote:
> > >>>>>
> > >>>>>> Hello there,
> > >>>>>> We have been extensively testing Netty connection versus NIIO and
> > >> there
> > >>>>>> are some issues that show up I wanted to get community response
> on.
> > >>>>>> In the process of testing https://issues.apache.
> > >>>>>> org/jira/browse/ZOOKEEPER-2509 fix we identified that
> sendResponse()
> > >>>>>> method may try to do some operations after close() was invoked -
> as
> > >>>>>> channel.close() in Netty is asynch. and subsequently lead to some
> > NPE.
> > >>>>>> NPE itself is not a good thing but the problems aggravates with
> the
> > >> fact
> > >>>>>> that propagation of NPE will lead to main processing thread
> exiting
> > >> and
> > >>>> at
> > >>>>>> that point ZK server becomes unresponsive - since no requests will
> > be
> > >>>>>> processed anymore.
> > >>>>>> In NIOServerCnxn.java in sendResponse() it is catching Exception
> and
> > >>>> just
> > >>>>>> logs a warning  which was added as part of
> > >>>> https://issues.apache.org/jira
> > >>>>>> /browse/ZOOKEEPER-597
> > >>>>>> I am trying to understand what a behavior should be in case of any
> > >>>>>> exception in sendResponse.
> > >>>>>> Any insight would be highly appreciated
> > >>>>>> Thanks,Yuliya
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>
> > >>
> > >
> > >
> > > --
> > > Cheers
> > > Michael.
> >
> >
>
>
> --
> Cheers
> Michael.
>
>
>
>

Reply via email to