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.

Sign up for GitHub

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

Jump to bottom

Add slf4j requst logger #3146

Merged
merged 5 commits into from
Jul 29, 2016
Merged

Add slf4j requst logger #3146

merged 5 commits into from
Jul 29, 2016

Conversation

drcrallen
Copy link
Contributor

@drcrallen drcrallen added this to the 0.9.2 milestone Jun 14, 2016
drcrallen
drcrallen reviewed Jun 14, 2016
View reviewed changes
server/src/main/java/io/druid/server/log/LoggingRequestLogger.java
LOG.info("%s", line);
}
finally {
if (setMDC && mdc != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might need to reset it regardless

Copy link
Contributor

@gianm gianm Jul 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think you're right. SLF4J docs say getCopyOfContextMap may return null, and the source makes it look like that will happen if nobody ever set up any MDC keys. So if mdc is null then it's probably best to call MDC.clear() to reset the MDC here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, also cleaned up code paths a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mdc is always fetched prior to logging, but only reset if setMDC was called

@gianm
Copy link
Contributor

gianm commented Jul 18, 2016

@drcrallen looks useful, left some comments in the diff.

@gianm
Copy link
Contributor

gianm commented Jul 26, 2016 •
edited
Loading

@drcrallen one of the tests isn't compiling, message is: [ERROR] /home/travis/build/druid-io/druid/server/src/test/java/io/druid/server/log/LoggingRequestLoggerTest.java:[183,1] io.druid.server.log.FakeQuery is not abstract and does not override abstract method getFilter() in io.druid.query.Query - method added in #2982.

👍 other than that

@drcrallen
Copy link
Contributor Author

@gianm fixed, thanks for the insight there

@gianm
Copy link
Contributor

gianm commented Jul 26, 2016

rad, thanks @drcrallen.

👍 after travis

@fjy
Copy link
Contributor

fjy commented Jul 29, 2016

👍

@fjy fjy merged commit d04af6a into apache:master Jul 29, 2016
@drcrallen drcrallen deleted the loggingRequetLogger branch July 29, 2016 22:38
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022
@navis
apache#3146 Fix exception getting object detail from S3
b8f5449
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
0.9.2
Development

Successfully merging this pull request may close these issues.
3 participants
@drcrallen @gianm @fjy