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 <[email protected]>
> 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 <[email protected]> wrote:
>>>
>>>> I'm very curious! Can't wait to see it. Go for it!
>>>>
>>>> On Sunday, January 26, 2014, Ralph Goers <[email protected]>
>>>> 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 <[email protected]>
>>>> 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 <[email protected]> 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 <[email protected]>
>>>>>> wrote:
>>>>>> On Fri, Jan 24, 2014 at 11:48 AM, Remko Popma <[email protected]>
>>>>>> 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 <[email protected]>
>>>>>> wrote:
>>>>>> On Thu, Jan 23, 2014 at 10:45 PM, Remko Popma <[email protected]>
>>>>>> 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 <[email protected]> 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: [email protected] | [email protected]
>>>>>> 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: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
