On Sat, Feb 13, 2016 at 7:47 AM, Ralph Goers <[email protected]> wrote:
> I didn’t create a branch because I didn’t think this was that big of a > deal. All I was doing was adding new methods to make entry and exit tracing > more usable. I created the MessageSupplier methods so the would be uniform > with the debug, trace, etc methods. I see they have now been modified to > use Supplier<Message> so that seems OK. > > I have a use case to log the response object as Json. It can be done in a > couple of ways: > > 1. logger.traceExit(response, gson->toJson(response)); > I could see this use case being needed if you want to configure Gson to pretty-print or not. Gary 2. logger.traceExit(response, new JsonMessage(response)); > > Personally, I like option 2 for this use case but I can imagine others > where option 1 might be preferred. Right now we have both a traceExit that > takes a MessageSupplier and one that takes Supplier<? extends Message>. > This doesn’t seem correct as their is no way to provide a Supplier that > allows option 1 above. That said, it appears I neglected to add that when I > first committed these changes. > > The ExitMessage adds “exit” to the start of the message just as the > EntryMessage adds “entry”. We always want those at the beginning of the > message so I don’t think ExitMessage should be removed. > > Ralph > > > > > > On Feb 13, 2016, at 4:38 AM, Remko Popma <[email protected]> wrote: > > > > On Saturday, 13 February 2016, Gary Gregory <[email protected]> > wrote: > >> Hi Remko, >> >> Thank you for your constructive feedback :-) >> >> On Sat, Feb 13, 2016 at 12:17 AM, Remko Popma <[email protected]> >> wrote: >> >>> In the future, can we make big changes like this on a separate branch >>> first so we can discuss them before they are committed to master? >>> >> >> I considered doing that, but the changes where much smaller than I >> initially thought. Also, did the new traceEntry/traceExit APIs come from a >> branch? Arg, did I miss that? I thought it would be OK to work on these new >> APIs in master since, well, they are new and I do not see a "1255" branch. >> Maybe it's been removed? >> > > There wasn't one. I guess the scope gradually increased during the course > of this Jira issue... > > >> >> >>> Some concrete feedback: >>> * Why does ExitMessage keep a reference to the result Object? It is not >>> used anywhere... (This could easily cause a memory leak...) >>> >> >> The Object is used to create the formatted string. >> > Missed that. Okay. > > >> >> It seems that most (all?) Message implementations compute their formatted >> strings on demand, this implementation does the same, hence the need to >> keep track of the Object. Since a Message is only created if a level+marker >> is enabled, it should be OK to compute formatted strings in ctors. We >> should discuss this/do it, through another email thread. Would you care to >> do the honors? But... computing these strings in ctors would make me want >> to have the ivar be final. Keeping in mind the no-GC epic, would we want a >> separate set of mutable messages? Or would we change the ones we have now >> to be more flexible to play in a no-GC use case. >> > > I'll update LOG4J2-1271 > <https://issues.apache.org/jira/browse/LOG4J2-1271> with details on why > ParameterizedMessage can be a bit thriftier. Not sure yet about what > other Message implementations to support for the no-GC epic. Entry and exit > currently need to walk the stacktrace to be useful (without this you don't > know _what_ method you entered/exited), which makes them extremely slow... > So I'm not considering them in scope for the no-GC epic. > > >> >>> * I don't see the point of AbstractMessage. The Message interface >>> already has a getFormattedMessage() method for obtaining the formatted >>> string. Why can't we call that method? >>> >> >> The point of AbstractMessage is that subclasses do not need to implement >> toString(). In order for messages to play nicely with MessageSupplier (or >> is Supplier), it needs a toString() to get the formatted String. See also >> the email thread about MS vs. S<?>. >> > > I saw the email thread but I didn't get it then either. All it said was "a > debug-oriented toString() does not make sense (to me at least, please see > the test cases I committed today and play around)", which didn't help me. > > Generally I'm against AbstractXxx classes unless there really is no better > way to do something. Classes like that encourage subclassing just to reuse > some trivial logic, not a good thing. There are other ways to accomplish > reuse, and I think sticking to just interfaces is cleaner. > > >> >> * Why does the responsibility of MessageFactory need to increase with >>> methods to create ExitMessage and EntryMessage? A separate >>> ExitMessageFactory/EntryMessageFactory seems more appropriate and flexible. >>> >> >> So org.apache.logging.log4j.spi.AbstractLogger would hold on to two >> message factories? a "plain" message factory and a "flow" message factory? >> Why? It seems perfectly natural for a "message factory" to produce all >> different kinds of messages. I do not see why having two factories is more >> flexible than one. >> >> >>> * typo: AbstactFlowMessage -> AbstractFlowMessage >>> >> >> Fixing... >> >> * typo: SimpleEntryMessage.DEAULT_TEXT -> DEFAULT_TEXT >>> >> >> Fixing... >> >> >>> * typo: SimpleExitMessage.DEAULT_TEXT -> DEFAULT_TEXT >>> >> >> Fixing... >> >> FYI: As of now, I do not have a use case for an ExitMessage but I thought >> it would be good for symmetry. But keeping YAGNI, we could just have >> EntryMessage extend Message and then remove FlowMessage and ExitMessage. >> > > If it can be removed we probably should. I agree with YAGNI: it might get > in the way of something else we want to do later. > > >> >> >>> Grumpily, >>> Remko >>> :-) >>> >> >> ;-) >> Gary >> >> >> >> -- >> E-Mail: [email protected] | [email protected] >> Java Persistence with Hibernate, Second Edition >> <http://www.manning.com/bauer3/> >> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >> Spring Batch in Action <http://www.manning.com/templier/> >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> > > -- E-Mail: [email protected] | [email protected] Java Persistence with Hibernate, Second Edition <http://www.manning.com/bauer3/> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> Spring Batch in Action <http://www.manning.com/templier/> Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
