Core: Migrating from joda to java.time. ML package #35493
Conversation
This commit moves the aggregation and mapping code from joda time to java time. This includes field mappers, root object mappers, aggregations with date histograms, query builders and a lot of changes within tests.
…m property configured
|
Pinging @elastic/es-core-infra |
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
4 times, most recently
from
055ec8d to
d3a6e85
Compare
There was a problem hiding this comment.
OK, I had a discussion with @jpountz and got to the bottom of this. BaseDateTime was the type returned for date fields in doc values prior to allowing configuring the doc value format. Thus, we still need this for mixed 6.x clusters. The comment below // script field is actually misleading. ML never supported scripted date fields.
There was a problem hiding this comment.
OK, I think the only thing left is to remove this TODO and correct the comment below (// script field). A more accurate comment would be // for handling doc value type prior to 6.4
There was a problem hiding this comment.
I need to investigate what will be the behaviour for a mix cluster like that.
Is there a test in any of the old branches that I can modify and see that place being hit? We could possibly then think of creating a similar one on the master branch with a mixed cluster?
There was a problem hiding this comment.
Actually we should be able to remove this from master. 7.x nodes should only talk to 6.latest which already have the doc value format specified. It's only in 6.x we need to keep this code for. Any datafeed test should be hitting this prior to 6.4.
There was a problem hiding this comment.
Note that prior to 6.4 there was no if block in this method. It was directly casting to the joda time.
There was a problem hiding this comment.
as we discussed, that sounds really good. Will remove that line in master
There was a problem hiding this comment.
Is this a question for the ML folk?
There was a problem hiding this comment.
@dimitris-athanasiou I thought initially to ask ML team, but from what I see now we are missing that migration in elasticsearch. This seems to be a planned work but not started yet.
Will have to switch to datetime histograms first before finishing ML package
There was a problem hiding this comment.
That is on the java-time branch at the moment and the ZonedDateTime will be returned
There was a problem hiding this comment.
Is this import used?
There was a problem hiding this comment.
unused indeed - will remove
There was a problem hiding this comment.
I believe this introduced a typo: keEy instead of key
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
from
3d5696f to
bb0d60b
Compare
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
from
bb0d60b to
d737fe5
Compare
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
from
41950c9 to
166be62
Compare
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
from
166be62 to
cef95b2
Compare
spinscale
left a comment
spinscale
left a comment
There was a problem hiding this comment.
left a few minor comments on how to get the current time in milliseconds, but it seems this has to wait for the java-time branch first
There was a problem hiding this comment.
how about Instant.now().toEpochMilli()?
There was a problem hiding this comment.
I think you are right, there is no point using ISOChronology when all we need here are milliseconds since epoch
There was a problem hiding this comment.
how about Instant.now().toEpochMilli()?
There was a problem hiding this comment.
how about Instant.now().toEpochMilli()?
There was a problem hiding this comment.
actually I ZonedDatetime, might better here - more on a new review https://github.com/elastic/elasticsearch/pull/35949/files#r236714576
|
@spinscale I will rebase this branch on top of java-time branch. It will help me to integrate when java-time is merged into master |
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
from
cef95b2 to
06b44dc
Compare
part of the migrating joda time work. The goal is to find usages of joda time and refactor to use java.time. closes: elastic#35490 confirmed histograms are returning zoneddatedate remove unused imports missing space typos removing the DateTime usage as it won't be hit removing tests no longer valid as code has been removed
pgomulka
force-pushed
the
feature/ML-java-time-ml-migration
branch
from
06b44dc to
3b90c7b
Compare
|
closing, as the change was merged into java-time |
4 participants
part of the migrating joda time work. The goal is to find usages of joda
time and refactor to use java.time.
refers #27330