By a generic definition, this new Level is certainly an enum. And it's written in Java. No, it's not a Java-language official enum. But, as far as byte code goes, it's nearly identical to an official enum. Users who don't need custom levels will use it EXACTLY like they would use an enum. It's an improvement over a traditional enum only in that you can extend it. I see no downsides at all.
There has obviously been some serious discussion about these topics. We're not going to come to a total agreement on this. I propose: - We have a committers-only vote in this thread on whether to make Level an extensible enum. - AFTER having that vote, we have a committers-only vote in the "Levels added in revision 1560602" thread on whether to add the three levels that were added. - We only roll back 1560602 AFTER the second vote is complete and IF the vote rejects the new levels. Nick On Jan 23, 2014, at 6:23 AM, Ralph Goers wrote: > I’m more in favor of this than what you had proposed. To be honest, with what > you proposed I don’t see much value left in keeping the Level enum. > > Yes, I prefer using the enum (obviously, or I wouldn’t have implemented it > that way), but if the consensus is to change it to a class, so be it. > > As for comments on the below, I think it needs a bit of tweaking (I wouldn’t > use Hashtable) but it may be a better option than adding more levels. Or we > could add the extra levels but not add new methods for them to AbstractLogger. > > Ralph > > On Jan 23, 2014, at 7:49 AM, Paul Benedict <[email protected]> wrote: > >> It's a neat idea but it's not a Java enum. I think one of Ralph's goal was >> to allow client code to use enums. I still think we should continue that >> path. At any rate, this will hopefully lead to a synthesis of ideas and >> agreement. >> >> >> On Thu, Jan 23, 2014 at 8:22 AM, Matt Sicker <[email protected]> wrote: >> Neat idea. I'd update it for proper concurrency, though. I could write a >> mock version of this to show what I mean. >> >> Matt Sicker >> >> > On Jan 23, 2014, at 2:42, Nick Williams <[email protected]> >> > wrote: >> > >> > Okay, I finally got a minute to read all of these emails, and... >> > >> > EVERYBODY FREEZE! >> > >> > What if I could get you an extensible enum that required no interface >> > changes and no binary-incompatible changes at all? Sound too good to be >> > true? I proposed this months ago (LOG4J2-41) and it got shot down multiple >> > times, but as of now I've heard THREE people say "extensible enum" in this >> > thread, so here it is, an extensible enum: >> > >> > public abstract class Level implements Comparable<Level>, Serializable { >> > public static final Level OFF; >> > public static final Level FATAL; >> > public static final Level ERROR; >> > public static final Level WARN; >> > public static final Level INFO; >> > public static final Level DEBUG; >> > public static final Level TRACE; >> > public static final Level ALL; >> > >> > >> > private static final long serialVersionUID = 0L; >> > private static final Hashtable<String, Level> map; >> > private static final TreeMap<Integer, Level> values; >> > private static final Object constructorLock; >> > >> > >> > static { >> > // static variables must be constructed in certain order >> > constructorLock = new Object(); >> > map = new Hashtable<String, Level>(); >> > values = new TreeMap<Integer, Level>(); >> > OFF = new Level("OFF", 0) {}; >> > FATAL = new Level("FATAL", 100) {}; >> > ERROR = new Level("ERROR", 200) {}; >> > WARN = new Level("WARN", 300) {}; >> > INFO = new Level("INFO", 400) {}; >> > DEBUG = new Level("DEBUG", 500) {}; >> > TRACE = new Level("TRACE", 600) {}; >> > ALL = new Level("ALL", Integer.MAX_VALUE) {}; >> > } >> > >> > >> > private static int ordinals; >> > >> > >> > private final String name; >> > private final int intLevel; >> > private final int ordinal; >> > >> > >> > protected Level(String name, int intLevel) { >> > if(name == null || name.length() == 0) >> > throw new IllegalArgumentException("Illegal null Level >> > constant"); >> > if(intLevel < 0) >> > throw new IllegalArgumentException("Illegal Level int less than >> > zero."); >> > synchronized (Level.constructorLock) { >> > if(Level.map.containsKey(name.toUpperCase())) >> > throw new IllegalArgumentException("Duplicate Level >> > constant [" + name + "]."); >> > if(Level.values.containsKey(intLevel)) >> > throw new IllegalArgumentException("Duplicate Level int [" >> > + intLevel + "]."); >> > this.name = name; >> > this.intLevel = intLevel; >> > this.ordinal = Level.ordinals++; >> > Level.map.put(name.toUpperCase(), this); >> > Level.values.put(intLevel, this); >> > } >> > } >> > >> > >> > public int intLevel() { >> > return this.intLevel; >> > } >> > >> > >> > public boolean isAtLeastAsSpecificAs(final Level level) { >> > return this.intLevel <= level.intLevel; >> > } >> > >> > >> > public boolean isAtLeastAsSpecificAs(final int level) { >> > return this.intLevel <= level; >> > } >> > >> > >> > public boolean lessOrEqual(final Level level) { >> > return this.intLevel <= level.intLevel; >> > } >> > >> > >> > public boolean lessOrEqual(final int level) { >> > return this.intLevel <= level; >> > } >> > >> > >> > @Override >> > @SuppressWarnings("CloneDoesntCallSuperClone") >> > public Level clone() throws CloneNotSupportedException { >> > throw new CloneNotSupportedException(); >> > } >> > >> > >> > @Override >> > public int compareTo(Level other) { >> > return intLevel < other.intLevel ? -1 : (intLevel > other.intLevel >> > ? 1 : 0); >> > } >> > >> > >> > @Override >> > public boolean equals(Object other) { >> > return other instanceof Level && other == this; >> > } >> > >> > >> > public Class<Level> getDeclaringClass() { >> > return Level.class; >> > } >> > >> > >> > @Override >> > public int hashCode() { >> > return this.name.hashCode(); >> > } >> > >> > >> > public String name() { >> > return this.name; >> > } >> > >> > >> > public int ordinal() { >> > return this.ordinal; >> > } >> > >> > >> > @Override >> > public String toString() { >> > return this.name; >> > } >> > >> > >> > public static Level toLevel(String name) { >> > return Level.toLevel(name, Level.DEBUG); >> > } >> > >> > >> > public static Level toLevel(String name, Level defaultLevel) { >> > if(name == null) >> > return defaultLevel; >> > name = name.toUpperCase(); >> > if(Level.map.containsKey(name)) >> > return Level.map.get(name); >> > return defaultLevel; >> > } >> > >> > >> > public static Level[] values() { >> > return Level.values.values().toArray(new >> > Level[Level.values.size()]); >> > } >> > >> > >> > public static Level valueOf(String name) { >> > if(name == null) >> > throw new IllegalArgumentException("Unknown level constant [" + >> > name + "]."); >> > name = name.toUpperCase(); >> > if(Level.map.containsKey(name)) >> > return Level.map.get(name); >> > throw new IllegalArgumentException("Unknown level constant [" + >> > name + "]."); >> > } >> > >> > >> > public static <T extends Enum<T>> T valueOf(Class<T> enumType, String >> > name) { >> > return Enum.valueOf(enumType, name); >> > } >> > >> > >> > // for deserialization >> > protected final Object readResolve() throws ObjectStreamException { >> > return Level.valueOf(this.name); >> > } >> > } >> > >> > Extending it is easy: >> > >> > public final class ExtendedLevels { >> > public static final Level MY_LEVEL = new Level("MY_LEVEL", 250) {}; >> > } >> > >> > I still and have ALWAYS believed this was the best option. If we used this >> > option, I would be fine with not adding any new Levels because I could add >> > them myself. >> > >> > Nick >> > >> >> On Jan 22, 2014, at 7:04 PM, Remko Popma wrote: >> >> >> >> This is only a problem for webapps, right? >> >> Putting log4j jars in WEB-INF/lib avoids that problem (different class >> >> loader). >> >> Apps that really want to share log4j jars with other apps would need to >> >> play nice. Such apps would do well to use a naming convention like Gary >> >> suggests. >> >> Otherwise, the last to register would overwrite any previous level with >> >> the same name. (Should probably emit a StatusLogger warning.) >> >> >> >> Same intLevel for different names should not be a problem. >> >> >> >> >> >> On Thursday, January 23, 2014, Gary Gregory <[email protected]> >> >> wrote: >> >> Playing devils advocate: >> >> >> >> What happens when different apps register levels with the same name and >> >> different intLevels? >> >> What happens when different apps register levels with the same intLevel >> >> and different names? >> >> Should there be a convention that custom level names be FQNs? >> >> >> >> Gary >> >> >> >> >> >> On Wed, Jan 22, 2014 at 10:05 PM, Paul Benedict <[email protected]> >> >> wrote: >> >> As Gary wanted, a new thread.... >> >> >> >> First, each enum needs an inherit strength. This would be part of the >> >> interface. Forgive me if the word "strength" is wrong; but it's the 100, >> >> 200, 300, etc. number that triggers the log level. So make sure the >> >> interface contains the intLevel() method. >> >> >> >> Second, we need to know the name, right? The name probably requires a new >> >> method since it can't be extracted from the enum anymore. >> >> >> >> public interface Level { >> >> int intLevel(); >> >> String name(); >> >> } >> >> >> >> PS: The intStrength() name seems hackish. What about strength() or >> >> treshold()? >> >> >> >> Third, the registration can be done manually by providing a static method >> >> (as your did Remko) that the client needs to invoke, or you could have a >> >> class-path scanning mechanism. For the latter, you could introduce a new >> >> annotation to be placed on the enum class. >> >> >> >> @CustomLevels >> >> public enum MyCustomEnums { >> >> } >> >> >> >> Paul >> >> >> >> On Wed, Jan 22, 2014 at 8:52 PM, Remko Popma <[email protected]> >> >> wrote: >> >> Paul, can you give a bit more detail? >> >> >> >> I tried this: copy the current Level enum to a new enum called "Levels" >> >> in the same package (other name would be fine too). Then change Level to >> >> an interface (removing the constants and static methods, keeping only the >> >> non-static methods). Finally make the Levels enum implement the Level >> >> interface. >> >> >> >> After this, we need to do a find+replace for the references to >> >> Level.CONSTANT to Levels.CONSTANT and Level.staticMethod() to >> >> Levels.staticMethod(). >> >> >> >> Finally, the interesting part: how do users add or register their custom >> >> levels and how do we enable the Levels.staticLookupMethod(String, Level) >> >> to recognize these custom levels? >> >> >> >> >> >> >> >> On Thursday, January 23, 2014, Paul Benedict <[email protected]> wrote: >> >> Agreed. This is not an engineering per se, but really more about if the >> >> feature set makes sense. >> >> >> >> Well if you guys ever look into the interface idea, you'll give log4j the >> >> feature of getting enums to represent custom levels. That's pretty cool, >> >> IMO. I don't know if any other logging framework has that and that would >> >> probably get some positive attention. It shouldn't be so hard to do a >> >> find+replace on the code that accepts Level and replace it with another >> >> name. Yes, there will be some minor refactoring that goes with it, but >> >> hard? It shouldn't be. >> >> >> >> A name I propose for the interface is LevelDefinition. >> >> >> >> Paul >> >> >> >> >> >> On Wed, Jan 22, 2014 at 6:48 PM, Gary Gregory <[email protected]> >> >> wrote: >> >> Hi, I do not see this as an engineering problem but more a feature set >> >> definition issue. So while there may be lots of more or less internally >> >> complicated ways of solving this with interfaces, makers and whatnots, >> >> the built in levels are the most user friendly. >> >> >> >> I have have lots of buttons, knobs and settings on my sound system that I >> >> do not use, just like I do not use all the methods in all the classes in >> >> the JRE... >> >> >> >> Gary >> >> >> >> >> >> >> >> >> >> -- >> >> 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] >> >> >> >> >> -- >> Cheers, >> Paul >
