Skip to content
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

LoggerFactory slf4j related regression fix #15659

Closed
wants to merge 4 commits into from

Conversation

yazun
Copy link

@yazun yazun commented Jul 27, 2023

Fix the regression that caused DRF loading fail due to spurious dependency on slf4j when log4j2 exists. Check for water logging availability was throwing exception if slf4j was not used while loading some models. Seems like a regression between 3.30 and 3.40. #6725

@wendycwong wendycwong requested a review from mmalohlava July 31, 2023 14:27
mn-mikke
mn-mikke previously approved these changes Aug 17, 2023
Copy link
Collaborator

@mn-mikke mn-mikke left a comment

Choose a reason for hiding this comment

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

@yazun The change looks good to me can you rebase your change against the branch rel-3.42.0 so we could release it ASAP?

Fix the regression that caused DRF loading fail due to spurious dependency on slf4j when log4j2 exists.
Check for water logging availability was throwing exception if slf4j was not used while loading some models. this seems like a regression between 3.30 and 3.40.
h2oai#6725
@yazun yazun force-pushed the LoggerFactory-slf4j-problem branch from 6d10f5b to 49cb990 Compare August 18, 2023 14:44
@yazun
Copy link
Author

yazun commented Aug 18, 2023

rebased to latest. I am on the road now with no local copy, so it might be done in 2 days or so.

@yazun
Copy link
Author

yazun commented Sep 19, 2023

Hello @mn-mikke
Should we do anything else - was it accepted?

@BerryHoll
Copy link

Dear @mmalohlava, would you have time to review this change? I work in the academic team with @yazun and currently we have a cumbersome workaround using a custom-build jar file by @yazun , but we would much prefer to migrate to the latest version that has this fix included. Best regards, and thanks!

@mn-mikke mn-mikke changed the base branch from master to rel-3.44.0 November 10, 2023 08:21
@mn-mikke mn-mikke dismissed their stale review November 10, 2023 08:21

The base branch was changed.

@mn-mikke mn-mikke changed the base branch from rel-3.44.0 to master November 10, 2023 08:22
@mn-mikke
Copy link
Collaborator

@yazun I've created a new PR which is rebased on rel-3.44.0 #15918 so the change would be shipped in the next fix release. Once it gets merged, the change will be automatically propagated to the master branch.

Sorry for the long delay. This PR got out of my attention.

@mn-mikke mn-mikke closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants