I completely agree. On Sunday, January 26, 2014, Gary Gregory <garydgreg...@gmail.com> wrote:
> It does not matter how rare you think a condition will be, with > concurrency issues, it's will happen, so we should only consider bug free > solutions. > > Gary > > > -------- Original message -------- > From: Remko Popma > Date:01/26/2014 00:13 (GMT-05:00) > To: Log4J Developers List > Subject: Re: Enums and Custom Levels > > Ralph, > I copied Nick's code _as is_ and had no compile errors. > The class is abstract, but instances are defined in the static block as: > OFF = new Level("OFF", 0) {}; // note the {}: this creates an anonymous > concrete subclass > > I agree that read access needs to be synchronized as well, not just write > access (the constructor). > I experimented with several options: > * synchronizing on plain Object in both constructor and when accessing the > static Map(s) > * a ReentrantReadWriteLock > * a lock-free implementation > > I decided against ReentrantReadWriteLock as it has more overhead than > plain synchronized access and the write access (in the constructors) is > going to be extremely rare: not worth paying the overhead in the more > common reads. It is also cumbersome to code. > > The lock-free implementation uses an AtomicInteger for the ordinals, and > an AtomicReference for the Map<String, Level>. > In the constructor, create a new Map<String, Level> instance based on the > old copy, add the new instance, and try to call compareAndSet to replace > the old instance with the new instance. Retry on failure. > > Finally, simply synchronizing on the constructorLock object in the > Level.toLevel() and Level.values() methods may be simplest. > > Which of the last two is best depends on how often the toLevel() and > values() levels are called. > It turns out they are only called during reconfiguration, so no real need > to optimize these methods. > I would argue that simple synchronization may be best in this case. > > Remko > > > > On Sun, Jan 26, 2014 at 1:49 PM, Ralph Goers > <ralph.go...@dslextreme.com<javascript:_e({}, 'cvml', > 'ralph.go...@dslextreme.com');> > > wrote: > >> As I am working on this I just want to point out a number of issues with >> the code below: >> >> 1. The class is abstract. The static block is doing a bunch of new >> Level() invocations which obviously generate compile errors on an abstract >> class. I had to make it be a non-abstract class. >> 2. As I pointed out before there is no way to access the “standard” >> levels as an enum. I have addressed that. >> 3. Although the constructor is synchronized access to the Map is not. >> Trying to get from the map while a Level is being added will result in a >> ConcurrentModificationException. I am using a ConcurrentMap instead. >> 3. The constructor requires synchronization because it is modifying both >> the map and the ordinal. However, since this isn’t an enum the ordinal >> value is of dubious value. Removing that would allow the removal of the >> synchronization in the constructor. I am considering that but I haven’t >> done it yet. >> 4. Your example of creating the extension shows doing a new Level(). This >> doesn’t work because a) the class is abstract and b) the constructor is >> protected. I am leaving the constructor protected so extension will require >> doing new ExtendedLevel(name, value) and creating a constructor. Not >> requiring that means applications can do a new Level() anywhere and I am >> opposed to allowing that. >> >> Ralph >> >> On Jan 23, 2014, at 12:42 AM, Nick Williams < >> nicho...@nicholaswilliams.net <javascript:_e({}, 'cvml', >> 'nicho...@nicholaswilliams.net');>> 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 >> >> <garydgreg...@gmail.com<javascript:_e({}, 'cvml', >> >> 'garydgreg...@gmail.com');>> >> 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 >> >> <pbened...@apache.org<javascript:_e({}, 'cvml', 'pbened...@apache.org');>> >> 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 >> >> <remko.po...@gmail.com<javascript:_e({}, 'cvml', >> >> 'remko.po...@gmail.com');>> >> 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 >> >> <pbened...@apache.org<javascript:_e({}, 'cvml', 'pbened...@apache.org');>> >> 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 >> >> <garydgreg...@gmail.com<javascript:_e({}, 'cvml', >> >> 'garydgreg...@gmail.com');>> >> 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: garydgreg...@gmail.com <javascript:_e({}, 'cvml', >> 'garydgreg...@gmail.com');> | ggreg...@apache.org <javascript:_e({}, >> 'cvml', '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<javascript:_e({}, 'cvml', >> > 'log4j-dev-unsubscr...@logging.apache.org');> >> > For additional commands, e-mail: >> > log4j-dev-h...@logging.apache.org<javascript:_e({}, 'cvml', >> > 'log4j-dev-h...@logging.apache.org');> >> > >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: >> log4j-dev-unsubscr...@logging.apache.org<javascript:_e({}, 'cvml', >> 'log4j-dev-unsubscr...@logging.apache.org');> >> For additional commands, e-mail: >> log4j-dev-h...@logging.apache.org<javascript:_e({}, 'cvml', >> 'log4j-dev-h...@logging.apache.org');> >> >> >