Gary - The name is very bad. Why anything named “getContextMap” would not return some form of the whole map is beyond me. Calling getContextMap(key) is just confusing. Why do you consider getContextMapValue “weird”? It seems perfectly appropriate to me. Also, I thought in another email you said getContextMap never returned null - or did I misread that?
Remko - when you say you don’t like it are you saying you are prepared to veto it? Ralph On May 16, 2014, at 4:21 PM, Remko Popma <[email protected]> wrote: > 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] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
