The idea is to make porting from 1.2 easy. Replacing getProperty with another name is easy. Yes, let's pick a better name. No we do not need the other API variants because 1.2 does not have them.
Gary <div>-------- Original message --------</div><div>From: Remko Popma <[email protected]> </div><div>Date:05/16/2014 19:21 (GMT-05:00) </div><div>To: List Log4J Developers <[email protected]> </div><div>Subject: Re: svn commit: r1595279 - in /logging/log4j/log4j2/trunk: log4j-core/src/main/java/org/apache/logging/log4j/core/ log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/ log4j-core/src/main/java/org/apache/logging/log4j/core/async/ log4j-c... </div><div> </div>Sorry, but I honestly don't like this change. First, you'll probably find that this is incomplete: the client code also likely needs to know the key values, so in future we'd have to add a "getContextKeys()" method. (Any api that has a "getValue(key)" method needs a "getKeys()" method too.) You mentioned you had trouble finding a good name, sometimes that is a sign that the method doesn't "belong" there. Finally it seems overkill to change an interface and four implementations to get a tiny benefit in one or two places. As an alternative, client code could extract the event's map into a local variable, and replace this local variable with Collections.EMPTY_MAP if it was null. From then on just use the local var without checking for null. Or we could change the semantics of event.getContextMap() to never return null. - Implementation note: if we do this, I'd prefer to do the null checking in the getter methods instead of in the constructor: keeping the instance variables null (when constructed with null) makes serialization faster. Sent from my iPhone > On 2014/05/17, at 3:14, [email protected] wrote: > > Author: ggregory > Date: Fri May 16 18:14:45 2014 > New Revision: 1595279 > > URL: http://svn.apache.org/r1595279 > Log: > Add LogEvent.getContextMap(String) to support easy ports from Log4j version > 1.2's getProperty(String) method. Since LogEvent.getContextMap() can return > null, it is cumbersome to use since a null check is required for bullet-proof > code, adding this 1-arg String method addresses the issue cleanly. > getContextMap(String) is not a great method name, but > getContextMapValue(String) seems weird and Log4j 1.2's getProperty(String) > name does not fit. > > Modified: > > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/AbstractLogEvent.java > > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/BasicLogEventEntity.java > > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEvent.java > > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java > > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/HTMLLayout.java > > logging/log4j/log4j2/trunk/log4j-flume-ng/src/main/java/org/apache/logging/log4j/flume/appender/FlumeEvent.java > > Modified: > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/AbstractLogEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/AbstractLogEvent.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/AbstractLogEvent.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/AbstractLogEvent.java > Fri May 16 18:14:45 2014 > @@ -37,6 +37,11 @@ public abstract class AbstractLogEvent i > } > > @Override > + public String getContextMap(String key) { > + return null; > + } > + > + @Override > public ContextStack getContextStack() { > return null; > } > > Modified: > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > Fri May 16 18:14:45 2014 > @@ -39,6 +39,15 @@ public interface LogEvent extends Serial > Map<String, String> getContextMap(); > > /** > + * Gets the value at the given key in the context map. > + * > + * @param key the key to query > + * @return the value to which the specified key is mapped, or {@code > null} if this map contains no mapping for the key or there is no > + * map. > + */ > + String getContextMap(String key); > + > + /** > * Gets the NDC data. > * > * @return A copy of the Nested Diagnostic Context or null; > @@ -160,4 +169,5 @@ public interface LogEvent extends Serial > * @see #getSource() > */ > void setIncludeLocation(boolean locationRequired); > + > } > > Modified: > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/BasicLogEventEntity.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/BasicLogEventEntity.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/BasicLogEventEntity.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/BasicLogEventEntity.java > Fri May 16 18:14:45 2014 > @@ -207,6 +207,18 @@ public abstract class BasicLogEventEntit > } > > /** > + * Gets the value at the given key in the context map. > + * > + * @param key the key to query > + * @return the value to which the specified key is mapped, or {@code > null} if this map contains no mapping for the key or there is no > + * map. > + */ > + @Override > + public String getContextMap(String key) { > + return this.getWrappedEvent().getContextMap(key); > + } > + > + /** > * Gets the context stack. Annotated with {@code @Convert(converter = > ContextStackAttributeConverter.class)}. > * > * @return the context stack. > > Modified: > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEvent.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEvent.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEvent.java > Fri May 16 18:14:45 2014 > @@ -190,6 +190,11 @@ public class RingBufferLogEvent implemen > return contextMap; > } > > + @Override > + public String getContextMap(String key) { > + return contextMap == null ? null : contextMap.get(key); > + } > + > @Override > public ContextStack getContextStack() { > return contextStack; > > Modified: > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java > Fri May 16 18:14:45 2014 > @@ -299,6 +299,18 @@ public class Log4jLogEvent implements Lo > } > > /** > + * Gets the value at the given key in the context map. > + * > + * @param key the key to query > + * @return the value to which the specified key is mapped, or {@code > null} if this map contains no mapping for the key or there is no > + * map. > + */ > + @Override > + public String getContextMap(String key) { > + return contextMap == null ? null : contextMap.get(key); > + } > + > + /** > * Returns the StackTraceElement for the caller. This will be the entry > that occurs right > * before the first occurrence of FQCN as a class name. > * @return the StackTraceElement for the caller. > > Modified: > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/HTMLLayout.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/HTMLLayout.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/HTMLLayout.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/HTMLLayout.java > Fri May 16 18:14:45 2014 > @@ -166,7 +166,7 @@ public final class HTMLLayout extends Ab > sbuf.append("</td>").append(Constants.LINE_SEPARATOR); > sbuf.append("</tr>").append(Constants.LINE_SEPARATOR); > > - if (event.getContextStack().getDepth() > 0) { > + if (event.getContextStack() != null && > !event.getContextStack().isEmpty()) { > sbuf.append("<tr><td bgcolor=\"#EEEEEE\" style=\"font-size : > ").append(fontSize); > sbuf.append(";\" colspan=\"6\" "); > sbuf.append("title=\"Nested Diagnostic Context\">"); > @@ -174,7 +174,7 @@ public final class HTMLLayout extends Ab > sbuf.append("</td></tr>").append(Constants.LINE_SEPARATOR); > } > > - if (event.getContextMap().size() > 0) { > + if (event.getContextMap() != null && > !event.getContextMap().isEmpty()) { > sbuf.append("<tr><td bgcolor=\"#EEEEEE\" style=\"font-size : > ").append(fontSize); > sbuf.append(";\" colspan=\"6\" "); > sbuf.append("title=\"Mapped Diagnostic Context\">"); > > Modified: > logging/log4j/log4j2/trunk/log4j-flume-ng/src/main/java/org/apache/logging/log4j/flume/appender/FlumeEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-flume-ng/src/main/java/org/apache/logging/log4j/flume/appender/FlumeEvent.java?rev=1595279&r1=1595278&r2=1595279&view=diff > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-flume-ng/src/main/java/org/apache/logging/log4j/flume/appender/FlumeEvent.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-flume-ng/src/main/java/org/apache/logging/log4j/flume/appender/FlumeEvent.java > Fri May 16 18:14:45 2014 > @@ -62,7 +62,7 @@ public class FlumeEvent extends SimpleEv > > private final LogEvent event; > > - private final Map<String, String> ctx = new HashMap<String, String>(); > + private final Map<String, String> contextMap = new HashMap<String, > String>(); > > private final boolean compress; > > @@ -95,7 +95,7 @@ public class FlumeEvent extends SimpleEv > for (String str : array) { > str = str.trim(); > if (mdc.containsKey(str)) { > - ctx.put(str, mdc.get(str)); > + contextMap.put(str, mdc.get(str)); > } > } > } > @@ -108,12 +108,12 @@ public class FlumeEvent extends SimpleEv > } > for (final Map.Entry<String, String> entry : mdc.entrySet()) { > if (!list.contains(entry.getKey())) { > - ctx.put(entry.getKey(), entry.getValue()); > + contextMap.put(entry.getKey(), entry.getValue()); > } > } > } > } else { > - ctx.putAll(mdc); > + contextMap.putAll(mdc); > } > > if (required != null) { > @@ -140,7 +140,7 @@ public class FlumeEvent extends SimpleEv > headers.put(GUID, guid); > } > > - addContextData(mdcPrefix, headers, ctx); > + addContextData(mdcPrefix, headers, contextMap); > } > > protected void addStructuredData(final String prefix, final Map<String, > String> fields, > @@ -291,7 +291,19 @@ public class FlumeEvent extends SimpleEv > */ > @Override > public Map<String, String> getContextMap() { > - return ctx; > + return contextMap; > + } > + > + /** > + * Gets the value at the given key in the context map. > + * > + * @param key the key to query > + * @return the value to which the specified key is mapped, or {@code > null} if this map contains no mapping for the key or there is no > + * map. > + */ > + @Override > + public String getContextMap(String key) { > + return contextMap == null ? null : contextMap.get(key); > } > > /** > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
