On Friday, 19 February 2016, Gary Gregory <[email protected]> wrote:
> On Thu, Feb 18, 2016 at 2:13 PM, Remko Popma <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> I think preserving binary compatibility on its own is a strong reason for >> doing this, but it's more than that. >> > > OK, since org.apache.logging.log4j.message.MessageFactory is in log4j-api > that's important. I can buy that. BUT, we are also adding methods to Logger > so that would break some things too. I guess less breakage is better than > more in this case! > That's a good point! Fortunately it's a lot easier to subclass AbstractLogger than implement all 200+ methods from scratch in a completely custom implementation of the Logger interface. :-) So I'm hoping that in practice this won't be an issue. > Overall, I not convinced that this is the best approach but I can > appreciate that you seem to feel about it stronger that I do. > > >> >> Having a separate factory for flow messages makes both factories more >> cohesive (single responsibility principle). No need for one factory to >> extend the other in my view. >> > > The distinction is pretty subtle here IMO. We are still talking about > creating messages, but I get your point. For me, the only reason for this > is to minimize the risk of API breakage, a nobe goal for the log4j-api > module, if not a requirement. > > >> >> The logger would have separate instances so users can configure them >> separately: lower coupling. >> > > OK. So now we have: > > org.apache.logging.log4j.LogManager.getLogger(Class<?>, MessageFactory) > org.apache.logging.log4j.LogManager.getLogger(Object, MessageFactory) > org.apache.logging.log4j.LogManager.getLogger(String, MessageFactory) > > We would add: > > org.apache.logging.log4j.LogManager.getLogger(Class<?>, MessageFactory, > FlowMessageFactory) > org.apache.logging.log4j.LogManager.getLogger(Object, MessageFactory, > FlowMessageFactory) > org.apache.logging.log4j.LogManager.getLogger(String, MessageFactory, > FlowMessageFactory) > > Right? Any other places? > I would start by making the default FlowMessageFactory configurable, so users can swap in their own if they want. Then if users express the need to configure FlowMessageFactories on a per-logger basis, add the methods to LogManager to support that. > >> >> These are both desirable properties so I believe it would improve the >> design. >> >> Does this make sense? >> > > Sure, even though I am less gun-ho about it than you are. I'd say go > ahead, see how it looks and feels after you refactor. We can keep > discussing it once your changes hits the repo if need be. > Cool. Thank you! -Remko > > Thank you for putting in the work! > Gary > > Remko >> >> Sent from my iPhone >> >> On 2016/02/19, at 2:24, Gary Gregory <[email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: >> >> Is a flow message factory a kind of message factory or a different kind >> of factory? >> >> Does a logger need instances of both or just the one? >> >> Since entry message extends message, should the factory do so as well? >> >> Gary, phone, typos. >> On Feb 18, 2016 8:44 AM, "Remko Popma" <[email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: >> >>> Would anyone mind terribly if I factored out the FlowMessage creation >>> methods from MessageFactory to a new interface FlowMessageFactory? >>> >>> Concretely, this interface would contain the methods introduced in >>> LOG4J2-1255: >>> >>> EntryMessage newEntryMessage(Message message); >>> ExitMessage newExitMessage(Object object, Message message); >>> ExitMessage newExitMessage(EntryMessage message); >>> ExitMessage newExitMessage(Object object, EntryMessage message); >>> >>> I think flow messages are different enough from normal Messages that a >>> separate factory makes sense. >>> >>> It would also insulate users who created a custom MessageFactory from the >>> changes we made in LOG4J2-1255. >>> >>> Thoughts? >>> >>> -Remko >>> >>> >>> > > > -- > 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 >
