I agree. The circumstance that could cause a problem here would be rare, but
not impossible. We should use Remko's code here.
Nick
On Jan 26, 2014, at 5:20 PM, Remko Popma wrote:
> 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 <[email protected]> wrote:
> +1 to everything Nick said.
>
>
> On 26 January 2014 16:40, Nick Williams <[email protected]> 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 <[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]
>
>
>
>
> --
> Matt Sicker <[email protected]>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
