I completely agree.
On Sunday, January 26, 2014, Gary Gregory <[email protected]> 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
> <[email protected]<javascript:_e({}, 'cvml',
> '[email protected]');>
> > 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 <
>> [email protected] <javascript:_e({}, 'cvml',
>> '[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]<javascript:_e({}, 'cvml',
>> >> '[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]<javascript:_e({}, 'cvml', '[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]<javascript:_e({}, 'cvml',
>> >> '[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]<javascript:_e({}, 'cvml', '[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]<javascript:_e({}, 'cvml',
>> >> '[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] <javascript:_e({}, 'cvml',
>> '[email protected]');> | [email protected] <javascript:_e({},
>> 'cvml', '[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]<javascript:_e({}, 'cvml',
>> > '[email protected]');>
>> > For additional commands, e-mail:
>> > [email protected]<javascript:_e({}, 'cvml',
>> > '[email protected]');>
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:
>> [email protected]<javascript:_e({}, 'cvml',
>> '[email protected]');>
>> For additional commands, e-mail:
>> [email protected]<javascript:_e({}, 'cvml',
>> '[email protected]');>
>>
>>
>
