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 <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> > --------------------------------------------------------------------- To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org