Proposed change to MarkerManager API

[Bruce Brouwer <br...@gmail.com>]

I hate changing API as much as the next guy, but there is an API in
MarkerManager that I think could be improved.

MarkerManager.getMarker(name) gets a marker, optionally creating it if it
doesn't exist. I support this with no change.

MarkerManager.getMarker(name, parents...) I have an issue with. It does not
in all cases return me a marker that has the parents specified. If the
marker already existed, it is simply returned with no changes made to the
parents. I propose removing this method and replacing it with...

MarkerManager.define(name, parents...) This method will create the marker
with the specified parents if it does not exist. If it does exist, it will
change the parent list to be the list specified.

Here's another reason I want to change this. Consider these two classes:

public class A {
  private final static Marker m = MarkerManager.getMarker("m", "p");
}

public class B {
  private final static Marker m = MarkerManager.getMarker("m");
}

If class A gets loaded first, marker "m" will have parent "p". But if class
B gets loaded first, then marker "m" will have no parents. I generally
don't like relying on the exact order that classes are loaded when the two
classes aren't related.

But if we consider my proposed change:

public class A {
  private final static Marker m = MarkerManager.define("m", "p");
}

public class B {
  private final static Marker m = MarkerManager.getMarker("m");
}

Now the behavior is the same, no matter which class is loaded first. It
looks clearer to me as the definition of the marker happens in one place,
while class B is only interested in a reference to a potentially already
defined marker, but makes no statement about what parents it has.

I also don't like the idea of the define method throwing an exception if
the marker already exists or getMarker returning null if the marker doesn't
exist; again for the reasons that I don't want class loading order to
impact behavior.

I understand that if there are two statements defining a marker with
different parents, whichever one runs last is going to be the winner. It's
not great if that happens, but I shouldn't be defining a marker in two
places. In reality, the best thing would be to make class B reference the
marker field in class A. If I do that, then the current implementation
would work fine; I'm just thinking of the case where somebody doesn't (or
can't) do the best thing. Then, a distinction between get and define could
be helpful.

Does this sound like an acceptable change that we could get into log4j-api
before GA?

-- 

Bruce Brouwer

Re: Proposed change to MarkerManager API

[Ralph Goers <ra...@dslextreme.com>]

I have mixed feelings about this.  I dislike the ambiguity of getMarker()  creating a Marker with the specified parents if it doesn’t exist or just returning the existing Marker, even if the parents are different.  However, I don’t really see anything in your proposal that fixes that.  I really dislike that the second cal to define would override the first. I believe it should throw an exception because what is happening is clearly wrong and should be fixed.

After thinking about this the only sensible thing I can think of is to get rid of the getMarker calls that accept parents and totally rely on the add method to create the parents.  That happens to also be how SLF4J works.  But, of course, that will break backward compatibility.

Ralph

On Apr 6, 2014, at 7:35 PM, Bruce Brouwer <br...@gmail.com> wrote:

> I hate changing API as much as the next guy, but there is an API in MarkerManager that I think could be improved. 
> 
> MarkerManager.getMarker(name) gets a marker, optionally creating it if it doesn't exist. I support this with no change.
> 
> MarkerManager.getMarker(name, parents...) I have an issue with. It does not in all cases return me a marker that has the parents specified. If the marker already existed, it is simply returned with no changes made to the parents. I propose removing this method and replacing it with...
> 
> MarkerManager.define(name, parents...) This method will create the marker with the specified parents if it does not exist. If it does exist, it will change the parent list to be the list specified. 
> 
> Here's another reason I want to change this. Consider these two classes:
> 
> public class A {
>   private final static Marker m = MarkerManager.getMarker("m", "p");
> }
> 
> public class B {
>   private final static Marker m = MarkerManager.getMarker("m");
> }
> 
> If class A gets loaded first, marker "m" will have parent "p". But if class B gets loaded first, then marker "m" will have no parents. I generally don't like relying on the exact order that classes are loaded when the two classes aren't related. 
> 
> But if we consider my proposed change:
> 
> public class A {
>   private final static Marker m = MarkerManager.define("m", "p");
> }
> 
> public class B {
>   private final static Marker m = MarkerManager.getMarker("m");
> }
> 
> Now the behavior is the same, no matter which class is loaded first. It looks clearer to me as the definition of the marker happens in one place, while class B is only interested in a reference to a potentially already defined marker, but makes no statement about what parents it has. 
> 
> I also don't like the idea of the define method throwing an exception if the marker already exists or getMarker returning null if the marker doesn't exist; again for the reasons that I don't want class loading order to impact behavior. 
> 
> I understand that if there are two statements defining a marker with different parents, whichever one runs last is going to be the winner. It's not great if that happens, but I shouldn't be defining a marker in two places. In reality, the best thing would be to make class B reference the marker field in class A. If I do that, then the current implementation would work fine; I'm just thinking of the case where somebody doesn't (or can't) do the best thing. Then, a distinction between get and define could be helpful.
> 
> Does this sound like an acceptable change that we could get into log4j-api before GA?
> 
> -- 
> 
> Bruce Brouwer


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org