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

Simplify initialization of LoggingHandler.logger. #703

Closed
wants to merge 2 commits into from

Conversation

benjaminp
Copy link

getLogging() was called in the constructor of LoggingHandler. So, there's no reason to have lazy initialization for the logger.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@benjaminp benjaminp requested review from a team as code owners September 30, 2021 18:48
@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/java-logging API. label Sep 30, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 30, 2021
@minherz minherz self-requested a review October 19, 2021 11:07
getLogging() was called in the constructor of LoggingHandler. So, there's no reason to have lazy initialization for the logger.
Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

we should make sure that after close() is called it is no longer possible to use the same logging instance

}
logging = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we still reset the logging instance here? since we do not throw exception we need a mechanism to avoid reuse of the potentially closed logging client.

Copy link
Author

Choose a reason for hiding this comment

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

The javadoc of Handler.close, which we're overriding here, says:

After close has been called this {@code Handler} should no longer be used.  Method calls may either be silently ignored or may throw runtime exceptions.

It's looks like a closed LoggingImpl will simply drop messages, meeting the behavior specified by the documentation.

@Neenu1995 Neenu1995 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Nov 1, 2021
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 1, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 1, 2021
@Neenu1995 Neenu1995 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 1, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 1, 2021
@product-auto-label product-auto-label bot added stale: old Pull request is old and needs attention. stale: critical Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Nov 26, 2021
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: critical Pull request is critically old and needs prioritization. labels Dec 8, 2021
@minherz
Copy link
Contributor

minherz commented Jan 1, 2022

The change of this PR is implemented in #812

@minherz minherz closed this Jan 1, 2022
@benjaminp benjaminp deleted the logging-handler branch January 2, 2022 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/java-logging API. cla: yes This human has signed the Contributor License Agreement. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants