zookeeper-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From yuliya Feldman <yufeld...@yahoo.com.INVALID>
Subject Re: Issue with NettyServerCnxn.java
Date Thu, 01 Sep 2016 20:17:00 GMT
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 <hanm@cloudera.com>
 To: UserZooKeeper <user@zookeeper.apache.org> 
Cc: dev@zookeeper.apache.org; yuliya Feldman <yufeldman@yahoo.com>; Patrick Hunt <phunt@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 <fpj@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 <hanm@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 <fpj@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 <breed@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
> >> <yufeldman@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 <breed@apache.org>
> >>>> To: Patrick Hunt <phunt@apache.org>
> >>>> Cc: DevZooKeeper <dev@zookeeper.apache.org>; yuliya Feldman <
> >>>> yufeldman@yahoo.com>; "user@zookeeper.apache.org" <
> >>>> user@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 <phunt@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 <
> >>>>> yufeldman@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.


   
Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message