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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');> | [email protected] > <javascript:_e(%7B%7D,'cvml','[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 >
