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" <[email protected]> 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 <[email protected]> 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:[email protected]] >>> Sent: Monday, April 04, 2011 7:20 AM >>> To: [email protected] >>> 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"<[email protected]> 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<[email protected]> wrote: >>>> >>>> Thanks for the info, Dan. Of c... >>>>> >>>> >>>> >>> >>> >>
