Logger: Merge ESLoggerFactory into Loggers #35146
Conversation
`ESLoggerFactory` is now not particularly interesting and simple enought to fold entirely into `Loggers. So let's do that. Closes elastic#32174
nik9000
added
:Core/Infra/Logging
|
Pinging @elastic/es-core-infra |
| static Logger getLogger(String prefix, Logger logger) { | ||
| /* | ||
| * In a followup we'll throw an exception if prefix is null or empty | ||
| * redirecting folks to LogManager.getLogger. |
There was a problem hiding this comment.
Does it make sense to already to add throwing that exception in this PR? I see the assertion added in PrefixLogger but that would only warn us in tests, right? I also checked #32174 for an open task of adding this Exception and couldn't find one, but maybe I missed it? If not, I think its worth adding it to not forget about it if this isn't done as part of this PR.
There was a problem hiding this comment.
I hadn't opened a task for it. To be honest I'd forgotten about it and saw it when doing this. I'm happy to switch the assert to a hard check.
|
Forgot one question about the intended 6.6 backport: as this is a breaking Java change, I was wondering if folks writing plugins etc. might currently use ESLoggerFactory in its current form and if we should deprecate on 6.6. instead of immediate removal. If that was the intention anyway then nevermind. |
|
About this being breaking for plugins: I'm lumping this in with the "our plugin API isn't stable" business. It is kind of a step to getting it stable because we no longer point folks to our own logging abstraction. But that is a fairly weak excuse to be honest. I think this is just what writing a plugin is like for now. One day, we'll have a stable plugin API. But we can't not change all this internal stuff just because plugins might touch it. |
fair enough |
…-agg * master: (528 commits) Register Azure max_retries setting (elastic#35286) add version 6.4.4 [Docs] Add painless context details for bucket_script (elastic#35142) Upgrade jline to 3.8.2 (elastic#35288) SQL: new SQL CLI logo (elastic#35261) Logger: Merge ESLoggerFactory into Loggers (elastic#35146) Docs: Add section about range query for range type (elastic#35222) [ILM] change remove-policy-from-index http method from DELETE to POST (elastic#35268) [CCR] Forgot missing return statement, SQL: Fix null handling for AND and OR in SELECT (elastic#35277) [TEST] Mute ChangePolicyForIndexIT#testChangePolicyForIndex Serialize ignore_throttled also to 6.6 after backport Check for java 11 in buildSrc (elastic#35260) [TEST] increase await timeout in RemoteClusterConnectionTests Add missing up-to-date configuration (elastic#35255) Adapt Lucene BWC version SQL: Introduce Coalesce function (elastic#35253) Upgrade to lucene-8.0.0-snapshot-31d7dfe6b1 (elastic#35224) Fix failing ICU tests (elastic#35207) Prevent throttled indices to be searched through wildcards by default (elastic#34354) ...
|
Thanks for reviewing @cbuescher! I finally finished the backport. |
`ESLoggerFactory` is now not particularly interesting and simple enought to fold entirely into `Loggers. So let's do that. Closes #32174
ESLoggerFactoryis now not particularly interesting and simple enoughtto fold entirely into `Loggers. So let's do that.
Closes #32174