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 <nicho...@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 <nicho...@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.po...@gmail.com> wrote:
>>>>
>>>>> I'm very curious! Can't wait to see it. Go for it!
>>>>>
>>>>> On Sunday, January 26, 2014, Ralph Goers <ralph.go...@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.go...@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.po...@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 <garydgreg...@gmail.com>
>>>>>>> wrote:
>>>>>>> On Fri, Jan 24, 2014 at 11:48 AM, Remko Popma <remko.po...@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 <garydgreg...@gmail.com>
>>>>>>> wrote:
>>>>>>> On Thu, Jan 23, 2014 at 10:45 PM, Remko Popma <remko.po...@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 <garydgreg...@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: garydgreg...@gmail.com | ggreg...@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-unsubscr...@logging.apache.org
>>> For additional commands, e-mail: log4j-dev-h...@logging.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
>> For additional commands, e-mail: log4j-dev-h...@logging.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
> For additional commands, e-mail: log4j-dev-h...@logging.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org
