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? > 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. 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 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<?>. * 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. > 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
