Add support for Log4j 2's ThreadContext

[Marcono1234]

Resolves #188

The code orientates itself to the existing SLF4J MDC one.

Noteworthy:

• Renames the SLF4J MDC module (but not its artifactId) to make the distinction between Log4j 1 & 2 (which use the term MDC as well) clearer
• Log4j 2 ThreadContext module currently supports Java 5 even though latest Log4j 2 versions require Java 8; this might allow usage with older Log4j 2 versions, but I have not tested it (relates to Next major release should have Java 8 as minimum version #112)
• Log4j 2 ThreadContextManager.clear() clears the ThreadContext of the current thread (see also discussion in SLF4J MDC context clear() does not clear context #204)
• Log4j 2 ThreadContextManager.getActiveContext().close() does nothing (similar to SLF4J MDC one)
• Propagating Log4j 2 ThreadContext to a different thread will add the data on top of the existing one instead of replacing it (see also discussion in Should ContextManager.initializeNewContext(T) always overwrite existing context? #202)

[sjoerdtalsma]

Thanks for your great work!
I did an initial review from the online github interface but will check out your branch to have a closer look from the IDE.

Please don't be overwhelmed by the number of comments; I tried to be constructive in my criticism and I like the general quality of this PR, which again is greatly appreciated!

[sjoerdtalsma]

Why package protected in a public class?
Can't the whole class be package protected in this case?

Please use an interface for the API if you don't want to expose the whole class; this makes it easier to avoid accessibility errors or misuse in the future.

[Marcono1234]

These methods are only intended for usage by ThreadContextManager. It might also be possible to have this logic only in ThreadContextManager, work with the public methods of ThreadContextData and have its constructor be package-private, but I felt the logic belongs closer to ThreadContextData than to ThreadContextManager.

As for why these methods are package-private: Most of them provide internal functionality already provided (indirectly) through ThreadContextManager or through Log4j 2's ThreadContext. Providing it publicly might be confusing and lead to incorrect usage.
Making the whole class package-private is not an option because it is publicly exposed through ThreadContextManager and would therefore cause usability issues when someone tries to use ThreadContextManager.

I don't think making it an interface fits really well because it is not expected that the user implements or subclasses this class. Because the package-private methods are not accessible to the user and are not shown in the javadoc either, I don't think that this is an issue.

Would it be alright for you to leave the class as is? If you want I can add a comment (not part of the documentation) to the class describing the rationale I described here.

[sjoerdtalsma]

When multiple context managers are applied it may be useful to know this is Log4j2's data?
Perhaps create an interface Log4j2ThreadContext with implementation Log4j2ThreadcontextData ?

[Marcono1234]

I am not sure if having an interface is worth it, see #207 (comment). We are not expecting the user to provide their own implementation of that class, are we?

[sjoerdtalsma]

Just to touch base, I have been rather busy lately with many things on my mind.
But let me ensure you, this PR has not been forgotten by me and it's on my list of things to do.

[sjoerdtalsma]

Could you please split this PR into the Log4j2 module only and (optionally) create separate PR's for the other concerns?
It is a bit much for me to review all in a single PR and I would like to be able to focus on the Log4j2 implementation, so we can have a clean first version.

[sjoerdtalsma]

I'll create a PR to rename the MDC module (with a new relocation in place for it)

[Marcono1234]

Could you please split this PR into the Log4j2 module only and (optionally) create separate PR's for the other concerns?

Yes, I can do that. Though if you don't mind, it would like to wait for your pull request to be merged first and then rebase this pull request onto develop.

[Marcono1234]

Sorry for the delay; I have rebased the changes onto #217 (so that should be merged first), and addressed some of your review comments (which I then marked as resolved).

[sjoerdtalsma]

Do you want to rebase on develop again? I merged #217 just now.
I think it would be a good idea to focus on the new log4j2 module and leave all else out of this PR.

[sjoerdtalsma]

I have some issues still, but I can work out those myself after I've merged this.
Let me thank you once more sincerely for your work and your patience with code review 😉.

[Marcono1234]

Thanks for merging the changes!

[sjoerdtalsma]

Thanks for merging the changes!

Thanks for contributing them!