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

feat: allow deployment time selection of logging framework #243 #244

Merged
merged 1 commit into from
May 1, 2024

Conversation

bestbeforetoday
Copy link
Contributor

@bestbeforetoday bestbeforetoday commented Apr 17, 2024

Depend on slf4j-api instead of the slf4j-jdk14 logging framework. This allows consumers to select their preferred logging framework at deployment time by adding an appropriate SLF4J provider to their runtime classpath.

If no SLF4J provider is available at runtime, no logging output will be produced. The code still functions as expected.

The isthmus CLI command continues to log using java.util.logging by including the slf4j-jdk14 provider in its runtime classpath.

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2024

CLA assistant check
All committers have signed the CLA.

@vbarua vbarua changed the title feat: Allow deployment time selection of logging framework feat: allow deployment time selection of logging framework Apr 17, 2024
@vbarua vbarua changed the title feat: allow deployment time selection of logging framework feat: allow deployment time selection of logging framework #243 Apr 17, 2024
@vbarua
Copy link
Member

vbarua commented Apr 17, 2024

@bestbeforetoday changes look reasonable to me.
You'll need to sign the CLA before we can get this merged in though.

@bestbeforetoday bestbeforetoday force-pushed the slf4j-api branch 2 times, most recently from 854a2f3 to bd6c5d6 Compare April 18, 2024 13:17
@bestbeforetoday
Copy link
Contributor Author

bestbeforetoday commented Apr 18, 2024

@vbarua Thank you for the quick feedback. I've made another pass to refine the change. I've still left the PR in draft as I need to get a review of the CLA from my employer before I can sign off on it. I'll mark it ready for review once I have that done. In the meantime, any feedback or change suggestions are very welcome.

@bestbeforetoday
Copy link
Contributor Author

@vbarua I have rebased on the latest commit and this should be ready for review now.

Depend on slf4j-api instead of the slf4j-jdk14 logging framework. This allows
consumers to select their preferred logging framework at deployment time by
adding an appropriate SLF4J provider to their runtime classpath.

If no SLF4J provider is available at runtime, no logging output will be
produced. The code still functions as expected.

The isthmus CLI command continues to log using java.util.logging by including
the slf4j-jdk14 provider in its runtime classpath.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for contributing this!

@vbarua vbarua merged commit 72bab63 into substrait-io:main May 1, 2024
8 checks passed
@bestbeforetoday bestbeforetoday deleted the slf4j-api branch May 1, 2024 20:48
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