Skip to content

Implement MessageFactory-namespaced logger registry #2936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Sep 16, 2024

Conversation

vy
Copy link
Member

@vy vy commented Sep 9, 2024

Fixes #2933 by deprecating AbstractLogger.checkMessageFactory() and removing its usages.

Rationale

In its current state, all LoggerContext#getLogger(String, MessageFactory) methods of all LoggerContext implementations delegate to LoggerRegistry, which in essence is a Map<MessageFactory, Map<String, Logger>>. That is, all LoggerRegistry-produced Loggers are MessageFactory-namespaced. This renders the checkMessageFactory() check redundant.

Put another way, getLogger("foo", someMF) will create a Logger associated with someMF. getLogger("foo") will create a Logger associated with the MF configured for the LC. In either case, if someMF != the MF configured for the LC, Loggers will differ anyway, hence checkMessageFactory() will always succeed.

@vy vy self-assigned this Sep 9, 2024
@vy vy requested review from rgoers and ppkarwasz September 9, 2024 20:19
@vy
Copy link
Member Author

vy commented Sep 9, 2024

@rgoers, would you mind reviewing this, please? (Checked with @ppkarwasz, he also agrees with deprecating checkMF().)

@vy vy changed the title Deprecate AbstractLogger.checkMessageFactory() Implement MessageFactory-namespaced logger registry Sep 11, 2024
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work Volkan!

This might be great for 2.25.0, but I am not sure the problem requires such a drastical changes. By this I mean the deprecations in LoggerRegistry.

Note also that all message factories should implement equals and hashCode (I think this is technically a minor version change).

@vy
Copy link
Member Author

vy commented Sep 11, 2024

This might be great for 2.25.0, but I am not sure the problem requires such a drastical changes. By this I mean the deprecations in LoggerRegistry.

I will merge these changes to 2.x (targeting 2.25.0) and then cherry-pick these to 2.24.x without deprecation changes.

Note also that all message factories should implement equals and hashCode (I think this is technically a minor version change).

AFAICT, LocalizedMessageFactory is the only stateful MF; implemented equals() and hashCode() for it in c747f4f.

vy and others added 2 commits September 16, 2024 12:42
Co-authored-by: Piotr P. Karwasz <piotr.github@karwasz.org>
@vy vy merged commit 724281c into 2.x Sep 16, 2024
6 checks passed
@vy vy deleted the feature/2.x/deprecate-AL-checkMF branch September 16, 2024 11:15
@vy vy added this to the 2.25.0 milestone Sep 16, 2024
@vy vy added bug Incorrect, unexpected, or unintended behavior of existing code api Affects the public API labels Sep 16, 2024
vy added a commit that referenced this pull request Sep 18, 2024
This feature will be released with `2.24.1` before `2.25.0` this
branch is pointing to. Hence, no need to duplicate entries.
@vy vy modified the milestones: 2.25.0, 2.24.1 Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API bug Incorrect, unexpected, or unintended behavior of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.24.0 status logger warns about message factory mismatch
2 participants