Fwd: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Volkan Yazıcı]

Hello,

Adding non-String-typed value support to MapMessage was also something on
my radar too. But this PR replacing String with Object in two lines seems
too good to be true to me. Does anybody mind taking a second look at this,
please?

Kind regards.

-- Forwarded message -
From: Henry Widd 
Date: Tue, Mar 23, 2021 at 4:58 PM
Subject: [apache/logging-log4j2] MapMessage put methods should not mandate
String values (#477)
To: apache/logging-log4j2 
Cc: Subscribed 


the underlying Map is typed  so the put methods on
MapMessage can also be.
--
You can view, comment on, or merge this pull request online at:

  https://github.com/apache/logging-log4j2/pull/477
Commit Summary

   - MapMessage put methods should not mandate String values

File Changes

   - *M*
   log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
   

   (6)

Patch Links:

   - https://github.com/apache/logging-log4j2/pull/477.patch
   - https://github.com/apache/logging-log4j2/pull/477.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
, or unsubscribe

.

release-2.x EventLogger

[Carter Kozak]

Any ideas why org.apache.logging.log4j.EventLoggerTest.structuredData would 
fail due to the following?

> java.lang.NullPointerException
> at 
> org.apache.logging.log4j.EventLoggerTest.structuredData(EventLoggerTest.java:42)

I haven't been able to reproduce this failure locally, but it appears to have 
occurred both on release-2.x and a PR branch I pushed later.

-Carter

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Matt Sicker]

Pretty sure that would break binary compatibility since it removes the
String method. I think it might be addable but not removed like that.

On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı  wrote:
>
> Hello,
>
> Adding non-String-typed value support to MapMessage was also something on
> my radar too. But this PR replacing String with Object in two lines seems
> too good to be true to me. Does anybody mind taking a second look at this,
> please?
>
> Kind regards.
>
> -- Forwarded message -
> From: Henry Widd 
> Date: Tue, Mar 23, 2021 at 4:58 PM
> Subject: [apache/logging-log4j2] MapMessage put methods should not mandate
> String values (#477)
> To: apache/logging-log4j2 
> Cc: Subscribed 
>
>
> the underlying Map is typed  so the put methods on
> MapMessage can also be.
> --
> You can view, comment on, or merge this pull request online at:
>
>   https://github.com/apache/logging-log4j2/pull/477
> Commit Summary
>
>- MapMessage put methods should not mandate String values
>
> File Changes
>
>- *M*
>log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
>
> 
>(6)
>
> Patch Links:
>
>- https://github.com/apache/logging-log4j2/pull/477.patch
>- https://github.com/apache/logging-log4j2/pull/477.diff
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> , or unsubscribe
> 
> .

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Ralph Goers]

I looked the other day and wondered the same thing Volkan did. There are no 
unit tests and the contributor didn’t even indicate that he had tried it.  

I was initially concerned that the underlying Map wouldn’t support it since it 
has StringMap in its name. It turns out the values are objects.

Technically I don’t think this would break compatibility. Any code referencing 
the put(String, String) would automatically map to put(String, Object). He 
didn’t modify the get method which would have broken compatibility.

Ralph

> On Mar 24, 2021, at 8:27 AM, Matt Sicker  wrote:
> 
> Pretty sure that would break binary compatibility since it removes the
> String method. I think it might be addable but not removed like that.
> 
> On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı  wrote:
>> 
>> Hello,
>> 
>> Adding non-String-typed value support to MapMessage was also something on
>> my radar too. But this PR replacing String with Object in two lines seems
>> too good to be true to me. Does anybody mind taking a second look at this,
>> please?
>> 
>> Kind regards.
>> 
>> -- Forwarded message -
>> From: Henry Widd 
>> Date: Tue, Mar 23, 2021 at 4:58 PM
>> Subject: [apache/logging-log4j2] MapMessage put methods should not mandate
>> String values (#477)
>> To: apache/logging-log4j2 
>> Cc: Subscribed 
>> 
>> 
>> the underlying Map is typed  so the put methods on
>> MapMessage can also be.
>> --
>> You can view, comment on, or merge this pull request online at:
>> 
>>  https://github.com/apache/logging-log4j2/pull/477
>> Commit Summary
>> 
>>   - MapMessage put methods should not mandate String values
>> 
>> File Changes
>> 
>>   - *M*
>>   log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
>>   
>> 
>>   (6)
>> 
>> Patch Links:
>> 
>>   - https://github.com/apache/logging-log4j2/pull/477.patch
>>   - https://github.com/apache/logging-log4j2/pull/477.diff
>> 
>> —
>> You are receiving this because you are subscribed to this thread.
>> Reply to this email directly, view it on GitHub
>> , or unsubscribe
>> 
>> .
> 

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Gary Gregory]

I thought this had to do with limiting garbage retention?

On Wed, Mar 24, 2021, 03:39 Volkan Yazıcı  wrote:

> Hello,
>
> Adding non-String-typed value support to MapMessage was also something on
> my radar too. But this PR replacing String with Object in two lines seems
> too good to be true to me. Does anybody mind taking a second look at this,
> please?
>
> Kind regards.
>
> -- Forwarded message -
> From: Henry Widd 
> Date: Tue, Mar 23, 2021 at 4:58 PM
> Subject: [apache/logging-log4j2] MapMessage put methods should not mandate
> String values (#477)
> To: apache/logging-log4j2 
> Cc: Subscribed 
>
>
> the underlying Map is typed  so the put methods on
> MapMessage can also be.
> --
> You can view, comment on, or merge this pull request online at:
>
>   https://github.com/apache/logging-log4j2/pull/477
> Commit Summary
>
>- MapMessage put methods should not mandate String values
>
> File Changes
>
>- *M*
>log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
><
> https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> >
>(6)
>
> Patch Links:
>
>- https://github.com/apache/logging-log4j2/pull/477.patch
>- https://github.com/apache/logging-log4j2/pull/477.diff
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> , or unsubscribe
> <
> https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> >
> .
>

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Matt Sicker]

Source compatibility versus binary compatibility. It's source
compatible, but not binary compatible. We could be more sure of this
if it's filed against release-2.x since that has all the apicmp
details.

On Wed, 24 Mar 2021 at 12:56, Gary Gregory  wrote:
>
> I thought this had to do with limiting garbage retention?
>
> On Wed, Mar 24, 2021, 03:39 Volkan Yazıcı  wrote:
>
> > Hello,
> >
> > Adding non-String-typed value support to MapMessage was also something on
> > my radar too. But this PR replacing String with Object in two lines seems
> > too good to be true to me. Does anybody mind taking a second look at this,
> > please?
> >
> > Kind regards.
> >
> > -- Forwarded message -
> > From: Henry Widd 
> > Date: Tue, Mar 23, 2021 at 4:58 PM
> > Subject: [apache/logging-log4j2] MapMessage put methods should not mandate
> > String values (#477)
> > To: apache/logging-log4j2 
> > Cc: Subscribed 
> >
> >
> > the underlying Map is typed  so the put methods on
> > MapMessage can also be.
> > --
> > You can view, comment on, or merge this pull request online at:
> >
> >   https://github.com/apache/logging-log4j2/pull/477
> > Commit Summary
> >
> >- MapMessage put methods should not mandate String values
> >
> > File Changes
> >
> >- *M*
> >log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
> ><
> > https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> > >
> >(6)
> >
> > Patch Links:
> >
> >- https://github.com/apache/logging-log4j2/pull/477.patch
> >- https://github.com/apache/logging-log4j2/pull/477.diff
> >
> > —
> > You are receiving this because you are subscribed to this thread.
> > Reply to this email directly, view it on GitHub
> > , or unsubscribe
> > <
> > https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> > >
> > .
> >

Re: release-2.x EventLogger

[Carter Kozak]

The flake appears to be resolved, just as mysteriously as it appeared.

-Carter

On Wed, Mar 24, 2021, at 10:28, Carter Kozak wrote:
> Any ideas why org.apache.logging.log4j.EventLoggerTest.structuredData would 
> fail due to the following?
> > java.lang.NullPointerException
> > at 
> > org.apache.logging.log4j.EventLoggerTest.structuredData(EventLoggerTest.java:42)
> 
> I haven't been able to reproduce this failure locally, but it appears to have 
> occurred both on release-2.x and a PR branch I pushed later.
> -Carter
> 

Re: release-2.x EventLogger

[Gary Gregory]

On Wed, Mar 24, 2021 at 5:03 PM Carter Kozak  wrote:
>
> The flake appears to be resolved, just as mysteriously as it appeared.

That, or you have special powers ;-)

G

>
> -Carter
>
> On Wed, Mar 24, 2021, at 10:28, Carter Kozak wrote:
> > Any ideas why org.apache.logging.log4j.EventLoggerTest.structuredData would 
> > fail due to the following?
> > > java.lang.NullPointerException
> > > at 
> > > org.apache.logging.log4j.EventLoggerTest.structuredData(EventLoggerTest.java:42)
> >
> > I haven't been able to reproduce this failure locally, but it appears to 
> > have occurred both on release-2.x and a PR branch I pushed later.
> > -Carter
> >

Re: release-2.x EventLogger

[Remko Popma]

Wild guess: multi-threading issue? (That's my go-to boogeyman)
:-)

On Thu, Mar 25, 2021 at 6:03 AM Carter Kozak  wrote:

> The flake appears to be resolved, just as mysteriously as it appeared.
>
> -Carter
>
> On Wed, Mar 24, 2021, at 10:28, Carter Kozak wrote:
> > Any ideas why org.apache.logging.log4j.EventLoggerTest.structuredData
> would fail due to the following?
> > > java.lang.NullPointerException
> > > at
> org.apache.logging.log4j.EventLoggerTest.structuredData(EventLoggerTest.java:42)
> >
> > I haven't been able to reproduce this failure locally, but it appears to
> have occurred both on release-2.x and a PR branch I pushed later.
> > -Carter
> >
>

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Remko Popma]

I called it StringMap because the keys must be Strings. Admittedly not a
great name. :-)

Not sure exactly, but there may be cases where this change could cause an
issue:
putAll(final Map map) ->
putAll(final Map map)

On Thu, Mar 25, 2021 at 2:12 AM Ralph Goers 
wrote:

> I looked the other day and wondered the same thing Volkan did. There are
> no unit tests and the contributor didn’t even indicate that he had tried
> it.
>
> I was initially concerned that the underlying Map wouldn’t support it
> since it has StringMap in its name. It turns out the values are objects.
>
> Technically I don’t think this would break compatibility. Any code
> referencing the put(String, String) would automatically map to put(String,
> Object). He didn’t modify the get method which would have broken
> compatibility.
>
> Ralph
>
> > On Mar 24, 2021, at 8:27 AM, Matt Sicker  wrote:
> >
> > Pretty sure that would break binary compatibility since it removes the
> > String method. I think it might be addable but not removed like that.
> >
> > On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı 
> wrote:
> >>
> >> Hello,
> >>
> >> Adding non-String-typed value support to MapMessage was also something
> on
> >> my radar too. But this PR replacing String with Object in two lines
> seems
> >> too good to be true to me. Does anybody mind taking a second look at
> this,
> >> please?
> >>
> >> Kind regards.
> >>
> >> -- Forwarded message -
> >> From: Henry Widd 
> >> Date: Tue, Mar 23, 2021 at 4:58 PM
> >> Subject: [apache/logging-log4j2] MapMessage put methods should not
> mandate
> >> String values (#477)
> >> To: apache/logging-log4j2 
> >> Cc: Subscribed 
> >>
> >>
> >> the underlying Map is typed  so the put methods on
> >> MapMessage can also be.
> >> --
> >> You can view, comment on, or merge this pull request online at:
> >>
> >>  https://github.com/apache/logging-log4j2/pull/477
> >> Commit Summary
> >>
> >>   - MapMessage put methods should not mandate String values
> >>
> >> File Changes
> >>
> >>   - *M*
> >>
>  log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
> >>   <
> https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> >
> >>   (6)
> >>
> >> Patch Links:
> >>
> >>   - https://github.com/apache/logging-log4j2/pull/477.patch
> >>   - https://github.com/apache/logging-log4j2/pull/477.diff
> >>
> >> —
> >> You are receiving this because you are subscribed to this thread.
> >> Reply to this email directly, view it on GitHub
> >> , or unsubscribe
> >> <
> https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> >
> >> .
> >
>
>
>

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Carter Kozak]

The method argument type changes are an ABI break, but I recall extending
MapMessage within a project in order to support more expressive types -- that
may have relied on dangerously casting the result of getIndexedReadOnlyStringMap
to an IndexedStringMap.
Including a safer Object-valued MapMessage subclass (with overloaded put 
methods)
sounds reasonable to me given at least two of us have run into this!

-Carter

On Wed, Mar 24, 2021, at 19:29, Remko Popma wrote:
> I called it StringMap because the keys must be Strings. Admittedly not a
> great name. :-)
> 
> Not sure exactly, but there may be cases where this change could cause an
> issue:
> putAll(final Map map) ->
> putAll(final Map map)
> 
> On Thu, Mar 25, 2021 at 2:12 AM Ralph Goers  >
> wrote:
> 
> > I looked the other day and wondered the same thing Volkan did. There are
> > no unit tests and the contributor didn’t even indicate that he had tried
> > it.
> >
> > I was initially concerned that the underlying Map wouldn’t support it
> > since it has StringMap in its name. It turns out the values are objects.
> >
> > Technically I don’t think this would break compatibility. Any code
> > referencing the put(String, String) would automatically map to put(String,
> > Object). He didn’t modify the get method which would have broken
> > compatibility.
> >
> > Ralph
> >
> > > On Mar 24, 2021, at 8:27 AM, Matt Sicker  > > > wrote:
> > >
> > > Pretty sure that would break binary compatibility since it removes the
> > > String method. I think it might be addable but not removed like that.
> > >
> > > On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı  > > >
> > wrote:
> > >>
> > >> Hello,
> > >>
> > >> Adding non-String-typed value support to MapMessage was also something
> > on
> > >> my radar too. But this PR replacing String with Object in two lines
> > seems
> > >> too good to be true to me. Does anybody mind taking a second look at
> > this,
> > >> please?
> > >>
> > >> Kind regards.
> > >>
> > >> -- Forwarded message -
> > >> From: Henry Widd  > >> >
> > >> Date: Tue, Mar 23, 2021 at 4:58 PM
> > >> Subject: [apache/logging-log4j2] MapMessage put methods should not
> > mandate
> > >> String values (#477)
> > >> To: apache/logging-log4j2  > >> >
> > >> Cc: Subscribed  > >> >
> > >>
> > >>
> > >> the underlying Map is typed  so the put methods on
> > >> MapMessage can also be.
> > >> --
> > >> You can view, comment on, or merge this pull request online at:
> > >>
> > >>  https://github.com/apache/logging-log4j2/pull/477
> > >> Commit Summary
> > >>
> > >>   - MapMessage put methods should not mandate String values
> > >>
> > >> File Changes
> > >>
> > >>   - *M*
> > >>
> >  log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
> > >>   <
> > https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> > >
> > >>   (6)
> > >>
> > >> Patch Links:
> > >>
> > >>   - https://github.com/apache/logging-log4j2/pull/477.patch
> > >>   - https://github.com/apache/logging-log4j2/pull/477.diff
> > >>
> > >> —
> > >> You are receiving this because you are subscribed to this thread.
> > >> Reply to this email directly, view it on GitHub
> > >> , or unsubscribe
> > >> <
> > https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> > >
> > >> .
> > >
> >
> >
> >
> 

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Remko Popma]

Instead of overloading the existing method(s), how about adding new methods
with a slightly different name that takes Object parameters?

On Thu, Mar 25, 2021 at 8:46 AM Carter Kozak  wrote:

> The method argument type changes are an ABI break, but I recall extending
> MapMessage within a project in order to support more expressive types --
> that
> may have relied on dangerously casting the result of
> getIndexedReadOnlyStringMap
> to an IndexedStringMap.
> Including a safer Object-valued MapMessage subclass (with overloaded put
> methods)
> sounds reasonable to me given at least two of us have run into this!
>
> -Carter
>
> On Wed, Mar 24, 2021, at 19:29, Remko Popma wrote:
> > I called it StringMap because the keys must be Strings. Admittedly not a
> > great name. :-)
> >
> > Not sure exactly, but there may be cases where this change could cause an
> > issue:
> > putAll(final Map map) ->
> > putAll(final Map map)
> >
> > On Thu, Mar 25, 2021 at 2:12 AM Ralph Goers  >
> > wrote:
> >
> > > I looked the other day and wondered the same thing Volkan did. There
> are
> > > no unit tests and the contributor didn’t even indicate that he had
> tried
> > > it.
> > >
> > > I was initially concerned that the underlying Map wouldn’t support it
> > > since it has StringMap in its name. It turns out the values are
> objects.
> > >
> > > Technically I don’t think this would break compatibility. Any code
> > > referencing the put(String, String) would automatically map to
> put(String,
> > > Object). He didn’t modify the get method which would have broken
> > > compatibility.
> > >
> > > Ralph
> > >
> > > > On Mar 24, 2021, at 8:27 AM, Matt Sicker  boards%40gmail.com>> wrote:
> > > >
> > > > Pretty sure that would break binary compatibility since it removes
> the
> > > > String method. I think it might be addable but not removed like that.
> > > >
> > > > On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı  >
> > > wrote:
> > > >>
> > > >> Hello,
> > > >>
> > > >> Adding non-String-typed value support to MapMessage was also
> something
> > > on
> > > >> my radar too. But this PR replacing String with Object in two lines
> > > seems
> > > >> too good to be true to me. Does anybody mind taking a second look at
> > > this,
> > > >> please?
> > > >>
> > > >> Kind regards.
> > > >>
> > > >> -- Forwarded message -
> > > >> From: Henry Widd  notifications%40github.com>>
> > > >> Date: Tue, Mar 23, 2021 at 4:58 PM
> > > >> Subject: [apache/logging-log4j2] MapMessage put methods should not
> > > mandate
> > > >> String values (#477)
> > > >> To: apache/logging-log4j2  >
> > > >> Cc: Subscribed  subscribed%40noreply.github.com>>
> > > >>
> > > >>
> > > >> the underlying Map is typed  so the put methods on
> > > >> MapMessage can also be.
> > > >> --
> > > >> You can view, comment on, or merge this pull request online at:
> > > >>
> > > >>  https://github.com/apache/logging-log4j2/pull/477
> > > >> Commit Summary
> > > >>
> > > >>   - MapMessage put methods should not mandate String values
> > > >>
> > > >> File Changes
> > > >>
> > > >>   - *M*
> > > >>
> > >
> log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
> > > >>   <
> > >
> https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> > > >
> > > >>   (6)
> > > >>
> > > >> Patch Links:
> > > >>
> > > >>   - https://github.com/apache/logging-log4j2/pull/477.patch
> > > >>   - https://github.com/apache/logging-log4j2/pull/477.diff
> > > >>
> > > >> —
> > > >> You are receiving this because you are subscribed to this thread.
> > > >> Reply to this email directly, view it on GitHub
> > > >> , or unsubscribe
> > > >> <
> > >
> https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> > > >
> > > >> .
> > > >
> > >
> > >
> > >
> >
>

Re: release-2.x EventLogger

[Ralph Goers]

I believe Matt made the tests in the API run in parallel. I’ve often wondered 
about that since LogManager is creating contexts meant to be shared across 
multiple threads. I have no idea how all that will behave in a multi-threaded 
environment since most unit tests create and destroy LoggerContexts.

Ralph

> On Mar 24, 2021, at 4:25 PM, Remko Popma  wrote:
> 
> Wild guess: multi-threading issue? (That's my go-to boogeyman)
> :-)
> 
> On Thu, Mar 25, 2021 at 6:03 AM Carter Kozak  wrote:
> 
>> The flake appears to be resolved, just as mysteriously as it appeared.
>> 
>> -Carter
>> 
>> On Wed, Mar 24, 2021, at 10:28, Carter Kozak wrote:
>>> Any ideas why org.apache.logging.log4j.EventLoggerTest.structuredData
>> would fail due to the following?
 java.lang.NullPointerException
at
>> org.apache.logging.log4j.EventLoggerTest.structuredData(EventLoggerTest.java:42)
>>> 
>>> I haven't been able to reproduce this failure locally, but it appears to
>> have occurred both on release-2.x and a PR branch I pushed later.
>>> -Carter
>>> 
>> 

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Ralph Goers]

I’m sure that will drive Gary nuts.  Let’s call the new method “put2()”.

Ralph

> On Mar 24, 2021, at 5:18 PM, Remko Popma  wrote:
> 
> Instead of overloading the existing method(s), how about adding new methods
> with a slightly different name that takes Object parameters?
> 
> On Thu, Mar 25, 2021 at 8:46 AM Carter Kozak  wrote:
> 
>> The method argument type changes are an ABI break, but I recall extending
>> MapMessage within a project in order to support more expressive types --
>> that
>> may have relied on dangerously casting the result of
>> getIndexedReadOnlyStringMap
>> to an IndexedStringMap.
>> Including a safer Object-valued MapMessage subclass (with overloaded put
>> methods)
>> sounds reasonable to me given at least two of us have run into this!
>> 
>> -Carter
>> 
>> On Wed, Mar 24, 2021, at 19:29, Remko Popma wrote:
>>> I called it StringMap because the keys must be Strings. Admittedly not a
>>> great name. :-)
>>> 
>>> Not sure exactly, but there may be cases where this change could cause an
>>> issue:
>>> putAll(final Map map) ->
>>> putAll(final Map map)
>>> 
>>> On Thu, Mar 25, 2021 at 2:12 AM Ralph Goers > >
>>> wrote:
>>> 
 I looked the other day and wondered the same thing Volkan did. There
>> are
 no unit tests and the contributor didn’t even indicate that he had
>> tried
 it.
 
 I was initially concerned that the underlying Map wouldn’t support it
 since it has StringMap in its name. It turns out the values are
>> objects.
 
 Technically I don’t think this would break compatibility. Any code
 referencing the put(String, String) would automatically map to
>> put(String,
 Object). He didn’t modify the get method which would have broken
 compatibility.
 
 Ralph
 
> On Mar 24, 2021, at 8:27 AM, Matt Sicker > boards%40gmail.com>> wrote:
> 
> Pretty sure that would break binary compatibility since it removes
>> the
> String method. I think it might be addable but not removed like that.
> 
> On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı > >
 wrote:
>> 
>> Hello,
>> 
>> Adding non-String-typed value support to MapMessage was also
>> something
 on
>> my radar too. But this PR replacing String with Object in two lines
 seems
>> too good to be true to me. Does anybody mind taking a second look at
 this,
>> please?
>> 
>> Kind regards.
>> 
>> -- Forwarded message -
>> From: Henry Widd > notifications%40github.com>>
>> Date: Tue, Mar 23, 2021 at 4:58 PM
>> Subject: [apache/logging-log4j2] MapMessage put methods should not
 mandate
>> String values (#477)
>> To: apache/logging-log4j2 > >
>> Cc: Subscribed > subscribed%40noreply.github.com>>
>> 
>> 
>> the underlying Map is typed  so the put methods on
>> MapMessage can also be.
>> --
>> You can view, comment on, or merge this pull request online at:
>> 
>> https://github.com/apache/logging-log4j2/pull/477
>> Commit Summary
>> 
>>  - MapMessage put methods should not mandate String values
>> 
>> File Changes
>> 
>>  - *M*
>> 
 
>> log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
>>  <
 
>> https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> 
>>  (6)
>> 
>> Patch Links:
>> 
>>  - https://github.com/apache/logging-log4j2/pull/477.patch
>>  - https://github.com/apache/logging-log4j2/pull/477.diff
>> 
>> —
>> You are receiving this because you are subscribed to this thread.
>> Reply to this email directly, view it on GitHub
>> , or unsubscribe
>> <
 
>> https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> 
>> .
> 
 
 
 
>>> 
>> 

Re: [apache/logging-log4j2] MapMessage put methods should not mandate String values (#477)

[Remko Popma]

Haha!
putObject?



> On Mar 25, 2021, at 11:39, Ralph Goers  wrote:
> 
> I’m sure that will drive Gary nuts.  Let’s call the new method “put2()”.
> 
> Ralph
> 
>> On Mar 24, 2021, at 5:18 PM, Remko Popma  wrote:
>> 
>> Instead of overloading the existing method(s), how about adding new methods
>> with a slightly different name that takes Object parameters?
>> 
>>> On Thu, Mar 25, 2021 at 8:46 AM Carter Kozak  wrote:
>>> 
>>> The method argument type changes are an ABI break, but I recall extending
>>> MapMessage within a project in order to support more expressive types --
>>> that
>>> may have relied on dangerously casting the result of
>>> getIndexedReadOnlyStringMap
>>> to an IndexedStringMap.
>>> Including a safer Object-valued MapMessage subclass (with overloaded put
>>> methods)
>>> sounds reasonable to me given at least two of us have run into this!
>>> 
>>> -Carter
>>> 
>>> On Wed, Mar 24, 2021, at 19:29, Remko Popma wrote:
 I called it StringMap because the keys must be Strings. Admittedly not a
 great name. :-)
 
 Not sure exactly, but there may be cases where this change could cause an
 issue:
 putAll(final Map map) ->
 putAll(final Map map)
 
 On Thu, Mar 25, 2021 at 2:12 AM Ralph Goers >> >
 wrote:
 
> I looked the other day and wondered the same thing Volkan did. There
>>> are
> no unit tests and the contributor didn’t even indicate that he had
>>> tried
> it.
> 
> I was initially concerned that the underlying Map wouldn’t support it
> since it has StringMap in its name. It turns out the values are
>>> objects.
> 
> Technically I don’t think this would break compatibility. Any code
> referencing the put(String, String) would automatically map to
>>> put(String,
> Object). He didn’t modify the get method which would have broken
> compatibility.
> 
> Ralph
> 
>> On Mar 24, 2021, at 8:27 AM, Matt Sicker >> boards%40gmail.com>> wrote:
>> 
>> Pretty sure that would break binary compatibility since it removes
>>> the
>> String method. I think it might be addable but not removed like that.
>> 
>> On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı >> >
> wrote:
>>> 
>>> Hello,
>>> 
>>> Adding non-String-typed value support to MapMessage was also
>>> something
> on
>>> my radar too. But this PR replacing String with Object in two lines
> seems
>>> too good to be true to me. Does anybody mind taking a second look at
> this,
>>> please?
>>> 
>>> Kind regards.
>>> 
>>> -- Forwarded message -
>>> From: Henry Widd >> notifications%40github.com>>
>>> Date: Tue, Mar 23, 2021 at 4:58 PM
>>> Subject: [apache/logging-log4j2] MapMessage put methods should not
> mandate
>>> String values (#477)
>>> To: apache/logging-log4j2 >> >
>>> Cc: Subscribed >> subscribed%40noreply.github.com>>
>>> 
>>> 
>>> the underlying Map is typed  so the put methods on
>>> MapMessage can also be.
>>> --
>>> You can view, comment on, or merge this pull request online at:
>>> 
>>> https://github.com/apache/logging-log4j2/pull/477
>>> Commit Summary
>>> 
>>> - MapMessage put methods should not mandate String values
>>> 
>>> File Changes
>>> 
>>> - *M*
>>> 
> 
>>> log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
>>> <
> 
>>> https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
>> 
>>> (6)
>>> 
>>> Patch Links:
>>> 
>>> - https://github.com/apache/logging-log4j2/pull/477.patch
>>> - https://github.com/apache/logging-log4j2/pull/477.diff
>>> 
>>> —
>>> You are receiving this because you are subscribed to this thread.
>>> Reply to this email directly, view it on GitHub
>>> , or unsubscribe
>>> <
> 
>>> https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
>> 
>>> .
>> 
> 
> 
> 
 
>>> 
> 
> 

Re: release-2.x EventLogger

[Matt Sicker]

Any tests that rely on shared state that can’t be safely multithreaded
needs to be categorized as such. I thought I covered everything in API
already, but perhaps not.

On Wed, Mar 24, 2021 at 21:38 Ralph Goers 
wrote:

> I believe Matt made the tests in the API run in parallel. I’ve often
> wondered about that since LogManager is creating contexts meant to be
> shared across multiple threads. I have no idea how all that will behave in
> a multi-threaded environment since most unit tests create and destroy
> LoggerContexts.
>
> Ralph
>
> > On Mar 24, 2021, at 4:25 PM, Remko Popma  wrote:
> >
> > Wild guess: multi-threading issue? (That's my go-to boogeyman)
> > :-)
> >
> > On Thu, Mar 25, 2021 at 6:03 AM Carter Kozak  wrote:
> >
> >> The flake appears to be resolved, just as mysteriously as it appeared.
> >>
> >> -Carter
> >>
> >> On Wed, Mar 24, 2021, at 10:28, Carter Kozak wrote:
> >>> Any ideas why org.apache.logging.log4j.EventLoggerTest.structuredData
> >> would fail due to the following?
>  java.lang.NullPointerException
> at
> >>
> org.apache.logging.log4j.EventLoggerTest.structuredData(EventLoggerTest.java:42)
> >>>
> >>> I haven't been able to reproduce this failure locally, but it appears
> to
> >> have occurred both on release-2.x and a PR branch I pushed later.
> >>> -Carter
> >>>
> >>
>
>
>