-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Composite request logger doesn't invoke @LifeCycleStart and @LifeCycleStop methods on its dependencies #6173
Conversation
Hi @samarthjain, thank you for the PR. Would you add some more details about this change to the PR description? It's not intuitive to me because it looks that any class annotated with |
Thanks for taking a look, @jihoonson. I have updated the PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @samarthjain! This PR looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little ugly. How about adding start
and stop
methods to the RequestLogger interface instead? It can be backwards compatible through default methods.
@gianm sounds good to me. |
…eStop methods on its dependencies
b916c42
to
a046325
Compare
Thanks for the reviews. I have updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest change looks good to me. @gianm do you have more comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good.
As an example, if composite request logger uses FileRequestLogger as one of the loggers to use, then we need to make sure that the FileRequestLogger is properly initialized by invoking a method annotated as @LifeCycleStart.
`
@LifecycleStart
public void start()
{
...
`
It seems like just annotating methods with LifeCycleStart/stop isn't enough. I am not too sure why but it looks like because I have configured composite request logging.
druid.request.logging.type=composing druid.request.logging.loggerProviders=[{"type":"file", "dir":"/logs/druid"}, {"type":"emit"}]
Without the change that this PR is proposing, logging using the FileRequestLogger fails with an NPE because fileWriter isn't initialized. Simply annotating a method in CompositeRequestLogger as @LifeCycleStart isn't enough either since it won't invoke the corresponding @LifeCycleStart method on the FileRequestLogger unless I call it explicitly.