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]
>
>
