Yeah, I missed the {} as I modified the class before I tried to compile it.
You can’t just use an AtomicInteger because you might be adding a duplicate to
the map, in which case you have incorrectly incremented the ordinal. I used
simple synchronization.
Unfortunately, because the extended levels are no longer enums that implement
an interface, the code I used to instantiate them during configuration doesn’t
work. Class.getEnumConstants was nice because it doesn’t through a
SecurityException and most of the other methods I could use do. So now I am
looking for another way. Everything else works though.
Ralph
On Jan 25, 2014, at 9:13 PM, Remko Popma <remko.po...@gmail.com> wrote:
> 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>
> 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>
> 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> 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>
> >> 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> 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> 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>
> >> 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 | 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
>
>
