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

Composite request logger doesn't invoke @LifeCycleStart and @LifeCycleStop methods on its dependencies #6173

Merged
merged 1 commit into from Aug 17, 2018

Conversation

samarthjain
Copy link
Contributor

@samarthjain samarthjain commented Aug 14, 2018

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()
{
...

  baseDir.mkdirs();
  MutableDateTime mutableDateTime = DateTimes.nowUtc().toMutableDateTime(ISOChronology.getInstanceUTC());
  mutableDateTime.setMillisOfDay(0);
  synchronized (lock) {
    currentDay = mutableDateTime.toDateTime(ISOChronology.getInstanceUTC());

    fileWriter = getFileWriter();
  }

`

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.

@jihoonson
Copy link
Contributor

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 LifecycleStart and LifecycleStop should be started/stopped automatically without this change if they are properly added to the Lifecycle.

@samarthjain
Copy link
Contributor Author

Thanks for taking a look, @jihoonson. I have updated the PR description.

Copy link
Contributor

@jihoonson jihoonson left a 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.

Copy link
Contributor

@gianm gianm left a 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.

@jihoonson
Copy link
Contributor

@gianm sounds good to me.

@samarthjain
Copy link
Contributor Author

Thanks for the reviews. I have updated the PR.

Copy link
Contributor

@jihoonson jihoonson left a 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?

Copy link
Contributor

@gianm gianm left a 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.

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.

None yet

4 participants