*static Map in the extensible enum class
On Sun, Jan 26, 2014 at 2:56 PM, Remko Popma <remko.po...@gmail.com> wrote:
> After thinking about it some more, the code generation could just access
> the static Map in the extensible enums class and find out what instances
> are registered. That would work but assumes that the configuration (or at
> least the initialization of the static Map in Levels) is complete before
> code generation.
>
>
> On Sun, Jan 26, 2014 at 2:49 PM, Remko Popma <remko.po...@gmail.com>wrote:
>
>> Did you mean that a custom Level implementation cannot extend StdLevel?
>> So a custom Level implementation would need to include the levels defined
>> in StdLevel (DEBUG, INFO, ...) in addition to the custom levels.
>> I think I see now.
>>
>> How much of a problem is this though? Or, do we need to start thinking
>> about the code generation bit in order to be able to decide if the
>> trade-off is worth it?
>>
>>
>>
>> On Sun, Jan 26, 2014 at 2:39 PM, Remko Popma <remko.po...@gmail.com>wrote:
>>
>>> I don't think client code can do new Level(){} as the constructor
>>> requires String and int arguments.
>>>
>>> By the way, I am unclear on what went wrong with the enum approach you
>>> originally took.
>>> You said:
>>> StdLevel isn’t a Level because it can’t extend it if it is an enum,
>>> so I can’t initialize the levels using that.
>>>
>>> I don't understand. StdLevel implements the Level interface, right? So
>>> what do you mean by "it can't extend it"?
>>>
>>> The reason I ask is that for the code generation, "real" java enums may
>>> be easier to deal with than the extensible enum approach.
>>>
>>>
>>> On Sun, Jan 26, 2014 at 2:30 PM, Ralph Goers <ralph.go...@dslextreme.com
>>> > wrote:
>>>
>>>> Out of curiosity, what exactly is the benefit of declaring the class
>>>> abstract when it has a protected constructor? It seems like all you are
>>>> accomplishing is making the instantiation syntax uglier. It also bothers me
>>>> that open code can just do a new Level(){} - which will do nothing but
>>>> cause problems. I’m beginning to think that we should require an
>>>> annotation on the Level declaration of the extension class to avoid that.
>>>>
>>>> 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
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
