Ok, thank you. I'll merge in a couple of hours.

Gary

On Sep 19, 2016 2:22 PM, "Ralph Goers" <ralph.go...@dslextreme.com> wrote:

> I took a look and it looks OK to me.
>
> On Sep 19, 2016, at 12:02 AM, Gary Gregory <garydgreg...@gmail.com> wrote:
>
> Great, thanks Ralph.
>
> Gary
>
> On Sun, Sep 18, 2016 at 11:14 PM, Ralph Goers <ralph.go...@dslextreme.com>
> wrote:
>
>> Yup to both. I’ll look at it tomorrow.
>>
>> Ralph
>>
>> On Sep 18, 2016, at 10:31 PM, Gary Gregory <garydgreg...@gmail.com>
>> wrote:
>>
>> Nevermind, I can just overwrite the name in the Node's attribute map...
>> that works. I'd like a code review before or after merging to master.
>>
>> Gary
>>
>> On Sun, Sep 18, 2016 at 9:43 PM, Gary Gregory <garydgreg...@gmail.com>
>> wrote:
>>
>>> Hm, but how? org.apache.logging.log4j.core.appender.AbstractAppender
>>> .name
>>> <http://org.apache.logging.log4j.core.appender.abstractappender.name/>
>>> is final and there is no Appender.setName(String). Surely, we should not
>>> use reflection...
>>>
>>> Gary
>>>
>>> On Sun, Sep 18, 2016 at 9:34 PM, Ralph Goers <ralph.go...@dslextreme.com
>>> > wrote:
>>>
>>>> I haven’t looked at your code but when you create the “real” appender
>>>> you need to change its name to match the name of the selector so that
>>>> AppenderRefs work.
>>>>
>>>> Ralph
>>>>
>>>> On Sep 18, 2016, at 9:24 PM, Gary Gregory <garydgreg...@gmail.com>
>>>> wrote:
>>>>
>>>> I've implemented a first cut in the branch LOG4J2-1597 but I think I
>>>> need some help to connect the final dot (or two).
>>>>
>>>> When I run the new unit test org.apache.logging.log4j.core.
>>>> appender.ScriptSelectorAppenderTest, the status logger shows:
>>>>
>>>> 2016-09-18 21:19:09,393 main ERROR Unable to locate appender "SelectIt"
>>>> for logger config "root"
>>>> 2016-09-18 21:19:09,465 main ERROR Unable to locate appender "SelectIt"
>>>> for logger config "root"
>>>> 2016-09-18 21:19:09,485 main ERROR Unable to locate appender "SelectIt"
>>>> for logger config "root"
>>>> 2016-09-18 21:19:09,505 main ERROR Unable to locate appender "SelectIt"
>>>> for logger config "root"
>>>>
>>>> Which initially makes sense: the appender created and returned by the
>>>> builder of "SelectIt" is really an appender named "List2".
>>>>
>>>> I tried to add a hack in org.apache.logging.log4j.core.
>>>> appender.ScriptSelector.Builder.build() to no avail:
>>>>
>>>>             // This feels like a hack and it does not work:
>>>>             configuration.getAppenders().put(name, appender);
>>>>
>>>> Any thoughts?
>>>>
>>>> Gary
>>>>
>>>> On Sat, Sep 17, 2016 at 9:48 AM, Ralph Goers <
>>>> ralph.go...@dslextreme.com> wrote:
>>>>
>>>>> See inline
>>>>>
>>>>> On Sep 16, 2016, at 10:31 PM, Gary Gregory <garydgreg...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> On Fri, Sep 16, 2016 at 8:38 PM, Ralph Goers <ralph.goers@dslextreme.
>>>>> com> wrote:
>>>>>
>>>>>> Gary,
>>>>>>
>>>>>> I have no problem with components that can be dumbed down to do
>>>>>> simple things. I do have a problem with components that only do simple
>>>>>> things because people will constantly asked to have them be enhanced.
>>>>>>
>>>>>> As for what you are proposing here, can I just say “No”?
>>>>>>
>>>>>
>>>>> Sure! :-) You can say whatever you want! :-)
>>>>>
>>>>>
>>>>>> Having the Appenders element deferred just smells to me and having an
>>>>>> arbitrary script there just seems weird to me. Does it even have a 
>>>>>> contract
>>>>>> or is it a free-for-all? How does it cause multiple appenders to be
>>>>>> initialized?
>>>>>>
>>>>>> I think the RoutingAppender is a more appropriate solution. However,
>>>>>> if you want to dumb it down a bit and turn it into an AppenderSelector 
>>>>>> I’d
>>>>>> be ok with that. However, it would still be fairly similar to the
>>>>>> RoutingAppender.
>>>>>>
>>>>>
>>>>> OK, so going back to one of your eariler messages:
>>>>>
>>>>> ==copy start==
>>>>>
>>>>> This sort of sounds like you want an Appender Selector, which would be
>>>>> an Appender that uses a Selector to figure out which Appender to delegate
>>>>> to. This is a bit like the PatternSelector. I would imagine it would make
>>>>> sense to implement AppenderSelectors and LayoutSelectors.  You probably
>>>>> would want to dynamically initialize the Appenders much like the
>>>>> RoutingAppender does.
>>>>>
>>>>> Maybe it would look like:
>>>>>
>>>>> <Appenders>
>>>>>   <ScriptSelector name=“" default=“”>
>>>>>      <Script language=“groovy”><![CDATA[
>>>>>          if (System.getProperty”os.name”).contains(“OS/390”)) then {
>>>>>              return “Socket”;
>>>>>          } else {
>>>>>              return “File”;
>>>>>          }
>>>>>      </Script>
>>>>>      <Appenders>
>>>>>          <SocketAppender name=“Socket” …/>
>>>>>          <FileAppender name=“File” …/>
>>>>>      </Appenders>
>>>>>   </ScriptSelector>
>>>>> </Appenders>
>>>>>
>>>>> The thing is that this script would run every time the Selector was
>>>>> accessed while it sounds like you would only want the script to run when
>>>>> the Selector is initialized. We could do that too but the script would 
>>>>> need
>>>>> to be declared in a property that would only be used when the selector is
>>>>> initialized. I would want to support being able to do both.
>>>>>
>>>>> ==copy end==
>>>>>
>>>>> This is indeed like the RoutingAppender _except_ that the whole point
>>>>> is to do the script selection on start up. When you say that you'd want it
>>>>> both ways, on start up and on each log event; what would the configuration
>>>>> difference look like?
>>>>>
>>>>> But.. "Appender that uses a Selector to figure out which Appender to
>>>>> delegate to" ... that is _so_ much like a RoutingAppender as to be
>>>>> redundant, no?
>>>>>
>>>>>
>>>>> The difference is that a AppenderSelector can just implement the
>>>>> Builder or Factory and invoke the script at that time to figure out which
>>>>> Appender to create. It then returns that Appender. So while the
>>>>> AppenderSelector is technically an Appender, it really is just an
>>>>> AppenderBuilder.  The RoutingAppender is a real Appender.
>>>>>
>>>>>
>>>>> What I want is for the script to determine which appender to use
>>>>> (once), and instantiate that appender (once). There is no need for one
>>>>> appender to delegate to another appender.
>>>>>
>>>>>
>>>>> And that is what I just described.
>>>>>
>>>>>
>>>>> The more general case is for the script to determine which appenders
>>>>> (plural) to use (once), and instantiate those appenders (plural) (once).
>>>>> There is no need for one appender to delegate to another appender list. I
>>>>> do not have a use case for this today, but I do for the one appender case.
>>>>>
>>>>>
>>>>> An AppenderSelector could only instantiate a single Appender, not a
>>>>> group. If you wanted multiple appenders dynamically created this way you
>>>>> would using multiple selectors. I’m not sure I see that as a drawback.
>>>>>
>>>>>
>>>>>
>>>>> My goal would be explained to a user like this: "This feature helps
>>>>> you build your configuration dynamically, all from the configuration file,
>>>>> to determine which appender(s) to configure. This is different from using 
>>>>> a
>>>>> RoutingAppender which creates a level of indirection and decides what to 
>>>>> do
>>>>> for each log event _at runtime_" Yes, this is a simpler explanation than
>>>>> also explaining the new role of scripts in the RoutingAppender but you get
>>>>> the idea.
>>>>>
>>>>> I am open different solutions that meet the goal of building the
>>>>> configuration dynamically, as if you'd done it in XML explicitly (or JSON)
>>>>> but does not end up with one appender delegating to another.
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> Gary
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> E-Mail: garydgreg...@gmail.com | ggreg...@apache.org
>>>> 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: garydgreg...@gmail.com | ggreg...@apache.org
>>> 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: garydgreg...@gmail.com | ggreg...@apache.org
>> 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: garydgreg...@gmail.com | ggreg...@apache.org
> 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
>
>
>

Reply via email to