Deprecate camel case date format #58909
Conversation
Camel case date formats are deprecated and snake case should be used instead.
pgomulka
added
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
| @@ -6,6 +6,10 @@ | |||
| <option name="HOST" value="localhost" /> | |||
| <option name="PORT" value="5005" /> | |||
| <option name="AUTO_RESTART" value="true" /> | |||
| <RunnerSettings RunnerId="Debug"> | |||
There was a problem hiding this comment.
yes, that file should not be modified.
| } | ||
|
|
||
| private void deprecate(String format) { | ||
| if (DEPRECATED_CAMEL_CASE_NAMES.contains(format)) { |
There was a problem hiding this comment.
Shouldn't all camelCase names be deprecated, in which case we can only log the deprecated message if the first condition matches above?
There was a problem hiding this comment.
yes, all camel case should be deprecated. It is just for some like iso8601, hour, year etc snake case and camel case is the same. We do not need to log the deprecation for these
There was a problem hiding this comment.
In that case perhaps we could not fill in the camelCaseName and condition on that being non null? It seems like we are doing extra work to analyze the casing of the name when we have already separated the ok (snake case) from the deprecated (camelCase).
There was a problem hiding this comment.
I see what you mean now. That's the smart way to do it - will fix!
| public class FormatNamesTests extends TestCase { | ||
|
|
||
| public void testDeprecation(){ | ||
|
|
There was a problem hiding this comment.
will remove this. I added deprecation test to https://github.com/elastic/elasticsearch/pull/58909/files#diff-198e1ff52598409599e5a427493f0227R386
| import org.apache.logging.log4j.LogManager; | ||
| import org.elasticsearch.common.logging.DeprecationLogger; | ||
| import org.elasticsearch.common.util.LazyInitializable; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public enum FormatNames { |
There was a problem hiding this comment.
for the record. this class was already used by Joda class. By mistake it was not used by DateFormatters
| // it results in errors sent to status logger and startup to fail. | ||
| // Hence a lazy initialization. | ||
| private static final LazyInitializable<DeprecationLogger, RuntimeException> deprecationLogger | ||
| = new LazyInitializable(() -> new DeprecationLogger(LogManager.getLogger(FormatNames.class))); |
There was a problem hiding this comment.
as per comment. Some classes - which are used before loading log4j config - are using DateFormat.forPattern to get a date format. see XContentElasticsearchExtension
This is fine, but if I try to create a logger, before logging config is loaded I will get an exception (no config, writing to status logger) this is later checked and startup will fail.
I am not sure we can move loading log4j config as early on startup.
At the same time, this logging is a temporary. We will remove it in 8 (unless we want some more detailed exception)
@rjernst WDYT?
@rjernst we already have a PR that forbids using camelCase names #59417 I can refactor that PR (#59417 against master) to deprecate, then cherry-pick to 7.x and finally remove it in master in another PR. |
|
A large reason why we try to make all PRs go through the master branch, even deprecations for features that will be removed in followups, is that the master history then contains the deprecation. If we make isolated changes directly to 7.x, those changes will never show up in the history within master. For example, let's say 2 major versions later, I am looking at the history of DateFormatters, trying to find when we deprecated camelCase format names. If I go look at master or 9.x, at the DateFormatters class, I would not find the deprecation, since it was only applied to 7.x. If instead the deprecation first goes to master, all future N.x branches will contain that history in DateFormatters, as well as master itself, so we regain the history. It's not that it disappears completely. The change still exists in git history, but if it doesn't exist within master's linear history, it is much more difficult to find. |
|
that totally makes sens to me now. thanks for clarifying. I will follow up with possible a different PR/force push on this one |
|
closing in favour of #59555 against master |
Camel case date formats are deprecated and snake case should be used instead.