logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ralph Goers <ralph.go...@dslextreme.com>
Subject Re: Enums and Custom Levels - completed.
Date Mon, 27 Jan 2014 00:09:47 GMT
I think this is very similar to my most recent commit.  Since you are OK with removing the
ordinal I am going to do that along with fix the problem Remko mentioned.

Ralph

On Jan 26, 2014, at 2:40 PM, Nick Williams <nicholas@nicholaswilliams.net> wrote:

> I would be OK with getting rid of the ordinal. It makes it less enum-like, but I agree
that the ordinal really has little purpose now. The intLevel is more important.
> 
> Here may be the best approach I can think of for calculating the StandardLevel-equivalent
on instantiation:
> 
>    public static Level OFF = new Level("OFF", 0, StandardLevel.OFF) {};
>    ...
>    public static Level ALL = new Level("ALL", Integer.MAX_VALUE, StandardLevel.ALL);
> 
>    ...
> 
>    private Level(String name, int intLevel, StandardLevel standardLevel) { // this is
the only c-tor standard levels use
>        // same logic as current constructor
>        this.standardLevel = standardLevel;
>    }
> 
>    protected Level(String name, int intLevel) { // this is the only c-tor custom levels
use
>        this(name, intLevel, Level.calculateStandardLevel(intLevel));
>    }
> 
>    public enum StandardLevel {
>        OFF, FATAL, ERROR, WARN, INFO, DEBUG, TRACE, ALL
>    }
> 
> Thoughts?
> 
> N
> 
> On Jan 26, 2014, at 4:02 PM, Ralph Goers wrote:
> 
>> I do have one other comment. You mention that the ordinal value isn’t guaranteed
because the levels might be instantiated in a different order each time.  An alternative wold
be to just get rid of the ordinal.  It isn’t used anywhere by anything and when custom values
are added they will be added after the standard levels, which is correct but might not be
what you would expect.  Eliminating that would allow the static initialization to stay as
it is and get rid of the need for synchronization in the constructor.
>> 
>> Ralph
>> 
>> On Jan 26, 2014, at 12:41 PM, Nick Williams <nicholas@nicholaswilliams.net>
wrote:
>> 
>>> Some (ok, a lot of) feedback:
>>> 
>>> - `private static ConcurrentMap<String, Level> levels` should be final.
>>> 
>>> - `private static Object constructorLock` should be final. In fact, code inspection
flags this as a warning since code synchronizes on it.
>>> 
>>> - The standard Level constants should be instantiated in a static initializer
like in my original code. Otherwise the order they are instantiated in is unpredictable, and
DEBUG (for example) may even have a different ordinal each time the JVM starts. 
>>> 
>>> - Level isn't abstract. However, you use `new Level("xxx", n) {}` (brackets)
for Level.OFF, ExtendedLevels.NOTE, and ExtendedLevels.DETAIL, but you use `new Level("xxx",
n)` (no brackets) for other levels. IMO, Level should be abstract (so that there's always
exactly one instance of every class that extends Level, just like a real enum), and the levels
should all use {} to construct. However, if we don't make Level abstract, then we should remove
{} from OFF, NOTE, and DETAIL because it's unnecessary.
>>> 
>>> - The way StdLevel appears in the middle of Level separates its static fields
from its instance fields, constructor, and methods. It makes it difficult to read. I have
to scroll pass StdLevel to see the rest of Level. Can we please move StdLevel to the bottom
of Level?
>>> 
>>> - This is more personal preference, but I don't like abbreviating things in class
names. What is Std? We may know, but does everyone? To someone unfamiliar with English, Standard
is easy to translate, but they have to first figure out that Std is short for Std. Can be
please un-abbreviate this to StandardLevel?
>>> 
>>> - I disagree with this approach:
>>> 
>>>          if (levels.containsKey(name)) {
>>>              Level level = levels.get(name);
>>>              if (level.intLevel() != intLevel) {
>>>                  throw new IllegalArgumentException("Level " + name + " has already
been defined.");
>>>              }
>>>              ordinal = level.ordinal;
>>>          }
>>> 
>>> This allows multiple Levels with the same name/intLevel to be instantiated, which
prevents equality testing (level == Level.OFF) from being 100% deterministic. It should really
be this:
>>> 
>>>          if (levels.containsKey(name)) {
>>>              throw new IllegalArgumentException("Level " + name + " has already
been defined.");
>>>          }
>>> 
>>> - I think we should make all of the methods of Level final. Custom levels shouldn't
be able to change their behavior, IMO. Alternatively, if we don't make Level abstract, we
should make it final.
>>> 
>>> - Level has no JavaDoc. The Level constants in Level have no JavaDoc. That needs
to be fixed.
>>> 
>>> - I'm still not convinced StdLevel/StandardLevel is necessary. It seems like
an anti-pattern to me. From what I can tell (please let me know if I'm missing something),
StdLevel's entire purpose is to allow us to still switch on the standard levels. I think that's
a bad reason to create an enum whose constants mirror Level. The primary problem with the
StdLevel is that the conversion from a Level to a StdLevel in many cases happens __on every
logging event__. That's going to be hugely inefficient. Really, when a custom Level is created,
it should be created with an equivalent standard Level (or StdLevel). There are several ways
to accomplish this that are open to discussion, but I don't think the current StdLevel.getStdLevel
is the right approach. One alternative:
>>> 
>>>  private final Level standardLevel;
>>> 
>>>  protected Level(String name, int level, Level mapToOtherFrameworksAs) {
>>>      this(name, level);
>>>      this.standardLevel = mapToOtherFrameworksAs;
>>>  }
>>> 
>>>  private Level(String name, int level) {
>>>      // same as now
>>>      this.standardLevel = this;
>>>  }
>>> 
>>>  public final Level getStandardLevel() {
>>>      return this.standardLevel;
>>>  }
>>> 
>>> This does, admittedly, have some problems. Another alternative that I could also
be happy with that keeps the StandardLevel enum:
>>> 
>>>  private final StandardLevel standardLevel;
>>> 
>>>  protected Level(String name, int level, StandardLevel mapToOtherFrameworksAs)
{
>>>      this(name, level);
>>>      this.standardLevel = mapToOtherFrameworksAs;
>>>  }
>>> 
>>>  private Level(String name, int level) {
>>>      // same as now
>>>      this.standardLevel = this;
>>>  }
>>> 
>>>  public final StandardLevel getStandardLevel() {
>>>      return this.standardLevel;
>>>  }
>>> 
>>> Nick
>>> 
>>> On Jan 26, 2014, at 1:38 PM, Ralph Goers wrote:
>>> 
>>>> I’ve committed the changes.  Take a look at ExtendedLevels.java, ExtendedLevelTest.java
and log4j-customLevel.xml in the log4j-core test directories to see how it works.
>>>> 
>>>> Ralph
>>>> 
>>>> On Jan 26, 2014, at 1:19 AM, Remko Popma <remko.popma@gmail.com> wrote:
>>>> 
>>>>> I'm very curious! Can't wait to see it. Go for it!
>>>>> 
>>>>> On Sunday, January 26, 2014, Ralph Goers <ralph.goers@dslextreme.com>
wrote:
>>>>> I have completed the work on custom levels.  It uses a variation of Nick’s
“extensible enum” class.  The major difference with what he proposed is that the custom
enums must be declared in a class annotated with @Plugin(name=“xxxx” category=“Level”)
for them to be usable during configuration.
>>>>> 
>>>>> Are their any objections to me checking this in?  I’ll be doing the
commit at around noon Pacific Daylight Time if I don’t hear any.
>>>>> 
>>>>> Ralph
>>>>> 
>>>>> 
>>>>> 
>>>>> On Jan 25, 2014, at 7:08 AM, Ralph Goers <Ralph.Goers@dslextreme.com>
wrote:
>>>>> 
>>>>>> I am working on the implementation of custom levels now.  I should
have it done today.
>>>>>> 
>>>>>> Ralph
>>>>>> 
>>>>>> On Jan 24, 2014, at 7:07 PM, Remko Popma <remko.popma@gmail.com>
wrote:
>>>>>> 
>>>>>>> What is the best way to make progress on the custom levels implementation?
>>>>>>> 
>>>>>>> Do we re-open LOG4J-41 or start a fresh Jira ticket? For implementation
ideas, do we attach files to Jira, or create a branch?
>>>>>>> 
>>>>>>> Remko
>>>>>>> 
>>>>>>> On Saturday, January 25, 2014, Gary Gregory <garydgregory@gmail.com>
wrote:
>>>>>>> On Fri, Jan 24, 2014 at 11:48 AM, Remko Popma <remko.popma@gmail.com>
wrote:
>>>>>>> Gary,
>>>>>>> 
>>>>>>> The hard-coded levels were proposed because it seemed that the
extensible enum idea raised by Nick was not going to be accepted.
>>>>>>> My original position was that Markers could fulfill the requirement
but Nick and yourself made it clear that this was not satisfactory.
>>>>>>> 
>>>>>>> With extensible enums and markers off the table it seemed that
the hard-coded levels was the only alternative, and discussion ensued about what these levels
should be called and what strength they should have.
>>>>>>> 
>>>>>>> During this discussion, several people, including me, repeatedly
expressed strong reservations about adding pre-defined levels, but by this time I think people
were thinking there was no alternative.
>>>>>>> 
>>>>>>> It looked like we were getting stuck, with half the group moving
in one direction ("add pre-defined levels!") and the other half wanting to move in another
direction ("don't add pre-defined levels!"). I asked that we re-reviewed our assumptions and
try to reach a solution that would satisfy all users. 
>>>>>>> 
>>>>>>> We then decided to explore the option of using extensible enums
again. This is still ongoing, but I haven't seen anyone arguing against this idea since we
started this thread.
>>>>>>> 
>>>>>>> Hard-coded levels and the extensible enum are different solutions
to the same problem.
>>>>>>> 
>>>>>>> Hello All:
>>>>>>> 
>>>>>>> Absolutely not. See my DEFCON example. 
>>>>>>> Talking about an "extensible enum" is mixing design and implementation,
we are talking about 'custom' and/or 'extensible' levels.
>>>>>>> Custom/Extensible levels can be designed to serve one or all
of:
>>>>>>> 
>>>>>>> - Allow inserting custom levels between built-in levels.
>>>>>>> - Allow for domain specific levels outside of the concept of
built-in levels, the DEFCON example.
>>>>>>> - Should the custom levels themselves be extensible?
>>>>>>> 
>>>>>>> Gary
>>>>>>> 
>>>>>>> The extensible enum solution satisfies all of us who are opposed
to adding pre-defined levels, while also satisfying the original requirement raised by Nick
and yourself. Frankly I don't understand why you would still want the pre-defined levels.
>>>>>>> 
>>>>>>> Remko
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On Sat, Jan 25, 2014 at 12:53 AM, Gary Gregory <garydgregory@gmail.com>
wrote:
>>>>>>> On Thu, Jan 23, 2014 at 10:45 PM, Remko Popma <remko.popma@gmail.com>
wrote:
>>>>>>> Gary, 
>>>>>>> 
>>>>>>> I think that's a very cool idea!
>>>>>>> Much more flexible, powerful and elegant than pre-defined levels
could ever be. 
>>>>>>> 
>>>>>>> As I wrote: "I am discussing custom levels here with the understanding
that this is a separate topic from what the built-in levels are."
>>>>>>> 
>>>>>>> I'm not sure why you want to make the features mutually exclusive.
(Some) others agree that these are different features.
>>>>>>> 
>>>>>>> I see two topics:
>>>>>>> 
>>>>>>> - What are the default levels for a 21st century logging framework.
Do we simply blindly copy Log4j 1? Or do we look at frameworks from different languages and
platforms for inspiration?
>>>>>>> - How (not if, I think we all agree) should we allow for custom
levels.
>>>>>>> 
>>>>>>> Gary
>>>>>>> 
>>>>>>> It definitely makes sense to design the extensible enum with
this potential usage in mind. 
>>>>>>> 
>>>>>>> Remko
>>>>>>> 
>>>>>>> 
>>>>>>> On Friday, January 24, 2014, Gary Gregory <garydgregory@gmail.com>
wrote:
>>>>>>> I am discussing custom levels here with the understanding that
this is a separate topic from what the built-in levels are. Here is how I convinced myself
that custom levels are a “good thing”.
>>>>>>> 
>>>>>>> No matter which built-in levels exits, I may want custom levels.
For example, I want my app to use the following levels DEFCON1, DEFCON2, DEFCON3, DEFCON4,
and DEFCON5. This might be for one part of my app or a whole subsystem, no matter, I want
to use the built-in levels in addition to the DEFCON levels. It is worth mentioning that if
I want that feature only as a user, I can “skin” levels in a layout and assign any label
to the built-in levels. If I am also a developer, I want to use DEFCON levels in the source
code.
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> At first, my code might look like:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> logger.log(DefconLevels.DEFCON5, “All is quiet”);
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Let’s put aside for now the type of DefconLevels.DEFCON* objects.
I am a user, and I care about my call sites.
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> What I really want of course is to write:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> defconLogger.defcon5(“All is quiet”)
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Therefore, I argue that for any “serious” use of a custom
level, I will wrap a Logger in a custom logger class providing call-site friendly methods
like defcon5(String).
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> So now, as a developer, all I care about is DefConLogger. It
might wrap (or subclass) the Log4J Logger, who knows. The implementation of DefConLogger is
not important to the developer (all I care is that the class has ‘defconN’ method) but
it is important to the configuration author. This tells me that as a developer I do not care
how DefConLogger is implemented, with custom levels, markers, or elves. However, as configuration
author, I also want to use DEFCON level just like the built-in levels.
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> The configuration code co
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> -- 
>>>>>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
>>>>>>> Java Persistence with Hibernate, Second Edition
>>>>>>> JUnit in Action, Second Edition
>>>>>>> Spring Batch in Action
>>>>>>> Blog: http://garygregory.wordpress.com 
>>>>>>> Home: http://garygregory.com/
>>>>>>> Tweet! http://twitter.com/GaryGregory
>>>>>> 
>>>>> 
>>>> 
>>> 
>>> 
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>> 
>> 
>> 
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
> For additional commands, e-mail: log4j-dev-help@logging.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org


Mime
View raw message