Hi Alberto, I don't really know if this change is good or bad but I do have some thoughts about logging and changing behavior like this.
First off, we might think about whether or not logging these warnings is currently expected behavior. Would removing/changing it cause surprise for any users upgrading to the version that removes it? Would it make things more difficult for the vendor or Apache Geode to provide support to a user having issues in FunctionExecution? It might make more sense to refactor the code that issues these warnings to use a pluggable mechanism for handling exceptions and error conditions in FunctionExecution? For example, you might consider introducing an SPI for a new FunctionExecutionErrorHandler. The default implementation would log the exceptions at warning level. Since it's an SPI (using Java Service Loader to load an implementation), you could provide your own implementation that does anything you want in response, including the possibility of logging it at debug level instead of warning. This also highlights a bigger issue in Apache Geode. There has never been a policy or even set of recommendations on the logging performed by Geode classes and components. There is also no standard or recommendations for formatting or wording of log statements. I believe the issue involving exception logging in FunctionExecution is a direct result of this. Geode needs detailed guidelines about log levels, log statement wording, inclusion of exception stacks, use of Markers, etc. Not just per class, but probably per Geode component or service or layer as well. Geode also needs some devs to spend time experimenting with specifying custom log4j2.xml configuration files, finding usability problems and hopefully improving how this currently works so that it becomes easy for users to customize what information is actually being logged. For example, there are problems with how Log4j is currently used by Geode such as using Markers at TRACE or DEBUG levels instead of at INFO level -- in order to enable Marker controlled logging, one would have to set the log-level to DEBUG and then also enable the Marker. Since we know DEBUG level already produces too much logging (much of it useless noise), this just exacerbates the problem when what is really wanted is well-behaved, consistent INFO level logging AND the ability to enable Marked controlled logging without having to use a finer log-level. It's possible the solution to Marker usage in Geode is to move Marker controlled log statements to INFO level. That way you could leave the logging at INFO level and just enable one or more Markers to increase logging from specific areas. I primarily wanted to point out that you may be trying to address a smaller part of the real problem here and that you should always consider pluggable mechanisms for customizing behavior rather than just considering changes that would directly alter the current behavior. Also, remember to think about whether a change like this would be best in a major, minor or patch release. Some users or the devs supporting them may rely on this information (I'm not saying they do in this case, just that it's possible). Thanks, Kirk On Thu, Apr 28, 2022 at 3:00 AM Alberto Gomez <alberto.go...@est.tech> wrote: > Hi Barry, > > If the exception is returned by passing it to the ResultCollector's > sendException() method then the exception is not logged. If the exception > is returned by passing it to the lastResult() method then the exception > (and the stack trace) is logged. I am assuming that when you say that the > exception is returned in its result is done by means of the sendException() > method. > > I agree with you that Geode must be consistent and if an exception is > thrown by the function, then the exception should be logged no matter if > isHA is returning false or true. Like you, I have also observed that when > isHA is returning false the exception is not logged. > > I also think it is worth to at least make this logging of the exception > configurable for those cases where functions prefer to throw the exception > instead of sending it and still do not want to see those exceptions logged. > > Thanks, > > Alberto > ________________________________ > From: Barry Oglesby <bogle...@vmware.com> > Sent: Thursday, April 28, 2022 2:32 AM > To: Alberto Gomez <alberto.go...@est.tech>; dev@geode.apache.org < > dev@geode.apache.org> > Subject: Re: [PROPOSAL] Remove warning logs from FunctionException > > A function can throw an exception and can also return an exception in its > result. I'm not sure I've seen too many functions where throwing an > exception is the expected result. In my very quick testing, I see the > exception and stack logged if the exception is thrown by the function but > not if the exception is returned. Are you seeing that same behavior, or are > both cases logging the exception? I also see the behavior you described > where isHA returning false does not log the exception. I guess I would say > if an exception is thrown in either case, it should be logged. If it is > returned in the result, it shouldn't. > > ________________________________ > From: Alberto Gomez <alberto.go...@est.tech> > Sent: Tuesday, April 5, 2022 4:03 AM > To: dev@geode.apache.org <dev@geode.apache.org>; Barry Oglesby < > bogle...@vmware.com> > Subject: Re: [PROPOSAL] Remove warning logs from FunctionException > > > ⚠ External Email > > Thanks for your proposal, Juan. > > I still think that it makes sense to remove these warning logs altogether. > Even if the stack trace is removed, the amount of logs could still be huge > if the operations received is high and the percentage of exceptions > significant. > > One more factor to add to this discussion is that these warning logs are > only generated if the function is HA. If the function returns false to > isHA() then the log does not appear. > > I would say this is one more reason in favor of removing these logs so > that the system is consistent. > > @Barrett Oglesby<mailto:bogle...@vmware.com> are you still in favor of > keeping these warning logs? > > More opinions on this topic are very welcome in order to be able to decide. > > Thanks, > > Alberto > ________________________________ > From: Ju@N <jujora...@gmail.com> > Sent: Wednesday, March 30, 2022 7:04 PM > To: dev@geode.apache.org <dev@geode.apache.org> > Subject: Re: [PROPOSAL] Remove warning logs from FunctionException > > Hello all, > > What about something in the middle?: log a WARNING level message stating > that the Function named XXX failed and also log the details (including the > stack trace) using DEBUG log level?. This would reduce the amount of logs > for functions that fail frequently, and will also allow the person > troubleshooting/debugging issues to easily tell that something is wrong > with function XXX. > Cheers. > > > > On Wed, 30 Mar 2022 at 17:52, Jacob Barrett <jabarr...@vmware.com> wrote: > > > > > > > On Mar 30, 2022, at 9:45 AM, Alberto Gomez <alberto.go...@est.tech > <mailto: > > alberto.go...@est.tech>> wrote: > > > > The idea would not be to remove the logs but rather to change the level > of > > these logs from warning to debug level. > > > > I agree, if any exception is expected as part user provided action it > > should not produce verbose logging. > > > > -Jake > > > > > > -- > Ju@N > > ________________________________ > > ⚠ External Email: This email originated from outside of the organization. > Do not click links or open attachments unless you recognize the sender. >
