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

Add SLF4J Simple Binding to sample application #835

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

kota2and3kan
Copy link
Contributor

@kota2and3kan kota2and3kan commented Mar 31, 2023

This PR adds slf4j-simple to the sample application for Getting Started with ScalarDB.

The current sample application doesn't include any logging libraries. So, it outputs some WARNING messages of SLF4J as follows.

$ ./gradlew run --args="-action charge -amount 1000 -to user1"

> Task :run
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

BUILD SUCCESSFUL in 1s
2 actionable tasks: 1 executed, 1 up-to-date

Sometimes, these messages make users confusing. Users think "Any problem occurs?" by seeing these messages.
(Actually, I got such inquiry/feedback from a user.)

So, I added slf4j-simple to remove these WARNING messages. After this fixing, the sample application doesn't output some WARNING messages as follows.

$ ./gradlew run --args="-action charge -amount 1000 -to user1"

BUILD SUCCESSFUL in 1s
2 actionable tasks: 1 executed, 1 up-to-date

I think this fixing can reduce users' confusion.


However, I would like to discuss this fix a little. I think there are some pros/cons as follows.

  • If we don't include some logging libraries, the sample application outputs WARNING messages. Those messages can tell users "ScalarDB uses SLF4J". In other words, we can tell users "You need to use some logging libraries that SLF4J supports in your application".

  • If we include some logging libraries in the sample application, there is a possibility that users cannot notice that "ScalarDB uses SLF4J". But, the same WARNING messages will be output when users create their own application with ScalarDB. So, finally, I think users can notice that when they create their own applications.

Regarding the above two patterns, I couldn't judge which is better. (We should output these WARNING messages or not.)
So, I would like to know your opinions.
What do you think?
(If we should not include some logging libraries in the sample application, I will close this PR.)

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

As for your concern, slf4j-simple would be enough for the simple CLI application. I think users will consider which logging library to use in their application and update the dependency when they want to output log messages to files or something (e.g. using Logback's FileAppender.)

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Taking care of logging libraries (including those used in dependencies) is the user's responsibility, so let's move forward with this change. Thank you.

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.
Adding a logging framework for a sample app makes sense to me too. As you said, it's more user-friendly for a sample app to see log messages of ScalarDB out of the box.

@kota2and3kan
Copy link
Contributor Author

kota2and3kan commented Apr 4, 2023

Thank you all for your opinions!
My concerns seem to be fine, so I will merge this PR.

@kota2and3kan kota2and3kan merged commit 4e7a861 into master Apr 4, 2023
11 checks passed
@brfrn169 brfrn169 added this to Done in 3.9.0 Apr 27, 2023
@brfrn169 brfrn169 deleted the update-getting-started-app branch May 2, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
3.9.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants