They can already do the same thing with loggers right?
Scott
On Jan 25, 2014 10:19 PM, "Ralph Goers" <ralph.go...@dslextreme.com> wrote:
> A malicious app could do
>
> for (int i=0; i < 100000; ++i) {
> new Level(“Level” + i, 1000 + i){};
> }
>
> Sure idiots can do lots of bad things but I don’t think Levels should be
> quite that flexible.
>
> Ralph
>
> On Jan 25, 2014, at 9: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
>>>
>>>
>>
>>
>
>
