Skip to content

Logging: Drop Settings from security logger get calls #33940

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

Merged
merged 3 commits into from
Sep 27, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 21, 2018

Settings is no longer required to get a Logger and we went to quite
a bit of effort to pass it to the Logger getters. This removes the
Settings logger lookups from the security and x-pack:core.

`Settings` is no longer required to get a `Logger` and we went to quite
a bit of effort to pass it to the `Logger` getters. This removes the
`Settings` from all of the logger fetches in security and x-pack:core.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@cbuescher
Copy link
Member

This must have fallen into some CI downtime, so no jenkins run was triggered yet, so:
@elasticmachine test this please

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@nik9000 I left a tiny question but in general LGTM

public ChainTransformFactory(Settings settings, TransformRegistry registry) {
super(Loggers.getLogger(ExecutableChainTransform.class, settings));
public ChainTransformFactory(TransformRegistry registry) {
super(Loggers.getLogger(ExecutableChainTransform.class));
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: in other instances you replaced Loggers.getLogger with LogManager.getLogger. Any advantaged of the later? If so, could this also be done here or doesn't it matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

I should have done it here, yeah. The advantage of the latter is that the former is deprecated. It mostly just delegates to the latter.

@nik9000
Copy link
Member Author

nik9000 commented Sep 26, 2018

@elasticmachine retest this please

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@nik9000 nik9000 merged commit 33a264a into elastic:master Sep 27, 2018
nik9000 added a commit that referenced this pull request Sep 28, 2018
`Settings` is no longer required to get a `Logger` and we went to quite
a bit of effort to pass it to the `Logger` getters. This removes the
`Settings` from all of the logger fetches in security and x-pack:core.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 28, 2018
* master:
  Use more precise does S3 bucket exist method (elastic#34123)
  LLREST: Introduce a strict mode (elastic#33708)
  [CCR] Adjust list retryable errors (elastic#33985)
  Fix AggregationFactories.Builder equality and hash regarding order (elastic#34005)
  MINOR: Remove some deadcode in NodeEnv and Related (elastic#34133)
  Rest-Api-Spec: Correct spelling in filter_path description (elastic#33154)
  Core: Don't rely on java time for epoch seconds formatting (elastic#34086)
  Retry errors when fetching follower global checkpoint. (elastic#34019)
  Watcher: Reenable watcher stats REST tests (elastic#34107)
  Remove special-casing of Synonym filters in AnalysisRegistry (elastic#34034)
  Rename CCR APIs (elastic#34027)
  Fixed CCR stats api serialization issues and (elastic#33983)
  Support 'string'-style queries on metadata fields when reasonable. (elastic#34089)
  Logging: Drop Settings from security logger get calls (elastic#33940)
  SQL: Internal refactoring of operators as functions (elastic#34097)
kcm pushed a commit that referenced this pull request Oct 30, 2018
`Settings` is no longer required to get a `Logger` and we went to quite
a bit of effort to pass it to the `Logger` getters. This removes the
`Settings` from all of the logger fetches in security and x-pack:core.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants