river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tom Hobbs <tvho...@googlemail.com>
Subject Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can terminate discovery notifier threads
Date Tue, 05 Apr 2011 08:09:16 GMT
I agree.  If your going to have a noisy log then you may as well have a
useful noisy log!

On 5 Apr 2011 08:40, "Dan Creswell" <dan.creswell@gmail.com> wrote:
> I think we should make the change to logging to avoid this confusion. It's
> straightforward to do, low impact and we needn't do it across the entire
> codebase all at once. Just make things better over time....
>
> On 4 April 2011 23:55, Patricia Shanahan <pats@acm.org> wrote:
>
>> I'm having a lot of trouble making up my mind about this. I understand
your
>> comment, but here is the other side of it.
>>
>> FINEST logging means that someone really wants to find out what is going
on
>> in this area, probably because it may be involved in some bug.
>>
>> As the code is now, the only indication of the Throwable will be a "null"
>> in the log message.
>>
>> If we are lucky, the programmer will notice the null, read the code, see
>> that it could be due to a throw as well as a null getLocator() result,
>> modify the logging code to report the Throwable, and the problem will
>> reproduce the same way on the next run. In that case, we will have only
>> wasted one run.
>>
>> If we are unlucky, the programmer will interpret "locator = null" in the
>> log as meaning that the locator was null at that point, and be
unnecessarily
>> confused.
>>
>> Patricia
>>
>>
>>
>> On 4/4/2011 8:14 AM, Christopher Dolan wrote:
>>
>>> In this specific case, the Throwable is not relevant and not worth
>>> logging because the locator is only used for a Level.FINEST log message!
>>> More context from LookupLocatorDiscover.Notifier.run(), where the
>>> catch(Throwable) is in the middle:
>>>
>>> /* Log the event info about the lookup(s) */
>>> if(firstListener&& (logger.isLoggable(Level.FINEST)) ) {
>>> String eType = (task.discard ?
>>> "discarded":"discovered");
>>> ServiceRegistrar[] regs = e.getRegistrars();
>>> logger.finest(eType+" event -- "+regs.length
>>> +" lookup(s)");
>>> Map groupsMap = e.getGroups();
>>> for(int i=0;i<regs.length;i++) {
>>> LookupLocator loc = null;
>>> try {
>>> loc = regs[i].getLocator();
>>> } catch (Throwable ex) { /* ignore */ }
>>> String[] groups = (String[])groupsMap.get(regs[i]);
>>> logger.finest(" "+eType+" locator = "+loc);
>>> if(groups.length == 0) {
>>> logger.finest(" "+eType
>>> +" group = NO_GROUPS");
>>> } else {
>>> for(int j=0;j<groups.length;j++) {
>>> logger.finest(" "+eType+" group["+j+"] "
>>> +"= "+groups[j]);
>>> }//end loop
>>> }//endif(groups.length)
>>> }//end loop
>>> }//endif(firstListener&& isLoggable(Level.FINEST)
>>>
>>>
>>> There's nearly identical code in LookupDiscover.Notifier.run()
>>>
>>> Chris
>>>
>>> -----Original Message-----
>>> From: Patricia Shanahan [mailto:pats@acm.org]
>>> Sent: Monday, April 04, 2011 7:20 AM
>>> To: dev@river.apache.org
>>> Subject: Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener
>>> can terminate discovery notifier threads
>>>
>>> I understand Tom's feeling against just logging, but I think it is
>>> probably the best option for now. Once we log, we should be able to find
>>>
>>> out if this is an issue, and if there are cases that are happening that
>>> would benefit from some other action.
>>>
>>> My really strong objection is to *silently* catching and carrying on.
>>> Partly, that is a result of having done a lot of debug, some of which
>>> was made unnecessarily difficult by software that destroyed clues.
>>>
>>> Patricia
>>>
>>>
>>> On 4/4/2011 2:15 AM, Tom Hobbs wrote:
>>>
>>>> You're right about InvocationHandler I should probably wake up before
>>>>
>>> I send
>>>
>>>> emails.
>>>>
>>>> If the spec says that all "good" code throws ServerError we can leave
>>>>
>>> that
>>>
>>>> Throwable catch in as well. This way we know that any of the latter
>>>>
>>> means a
>>>
>>>> dos attack, non spec compliant services or something equally awful.
>>>>
>>>> I'm really reluctant to just leave a log and Throwable catch in; it
>>>>
>>> just
>>>
>>>> feels wrong. I guess we might have to though since writing code for
>>>>
>>> this
>>>
>>>> level requires a slightly different way of think than when at the
>>>> application level. I'm not going to keep flogging this dead horse
>>>>
>>> though, I
>>>
>>>> trust your judgement on this more than mine. :-)
>>>>
>>>> Tom
>>>>
>>>> On 4 Apr 2011 09:44, "Dan Creswell"<dan.creswell@gmail.com> wrote:
>>>> Can't do anything about the Throwable as it's part of
>>>>
>>> InvocationHandler and
>>>
>>>> that's the JDK spec.
>>>>
>>>> Could agree that our Dispatcher's only ever throw some specific
>>>>
>>> subclasses.
>>>
>>>> We'd have to do some diligence on that as BasicInvocationDispatcher
>>>>
>>> and
>>>
>>>> friends are designed to follow RMI spec, not entirely sure all other
>>>> transports can do enough in that respect to be compliant.
>>>>
>>>> There is one other problem with this however which is that badly
>>>>
>>> written
>>>
>>>> service code could chuck out stuff that is not compliant and bring
>>>>
>>> down the
>>>
>>>> entire house - that's kind of denial of service territory....
>>>>
>>>> ...personally I'd rather leave the catch throwable, log at some
>>>>
>>> suitable
>>>
>>>> level and leave it at that, at least until we gather some data as to
>>>>
>>> how
>>>
>>>> often this problem bites us etc.
>>>>
>>>>
>>>> On 4 April 2011 09:07, Tom Hobbs<tvhobbs@googlemail.com> wrote:
>>>>
>>>> Thanks for the info, Dan. Of c...
>>>>>
>>>>
>>>>
>>>
>>>
>>

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