By the way, there may be a small concurrency bug in the Level.values()
static method. This may be better:
public static Level[] values() {
Collection<Level> values = Level.levels.values();
return values.toArray(new Level[values.size()]); // use value
collection size, not map size
}
On Mon, Jan 27, 2014 at 7:43 AM, Matt Sicker <boa...@gmail.com> wrote:
> +1 to everything Nick said.
>
>
> On 26 January 2014 16:40, 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
>>
>>
>
>
> --
> Matt Sicker <boa...@gmail.com>
>
