Yay! Let's make getContextMap never return null then and remove getContextMapValue.
On Sat, May 17, 2014 at 8:49 PM, Gary Gregory <[email protected]>wrote: > If we document and enforce that getContextMap never returns null in all > our impls then we can loose getContextMapValue. > > Gary > > > -------- Original message -------- > From: Remko Popma > Date:05/17/2014 04:48 (GMT-05:00) > To: Log4J Developers List > 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... > > Ralph, I found the previous veto/not veto experience very uncomfortable > and I would much prefer some discussion first before driving it to that > extreme. > > Gary, I could not find any callers for this new method {{String > LogEvent#getContextMap(String)}}. Does this mean that the problem has > already been resolved? Perhaps this was resolved by changing the semantics > of {{Map<String, String> LogEvent#getContextMap()}} to always return a > non-null map? Now that this method is not used and there is no need for it > any more, can this method be removed? > > > On Sat, May 17, 2014 at 3:47 PM, Ralph Goers > <[email protected]>wrote: > >> 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] >> >> >
