-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Migrating from joda to java.time. Watcher plugin #35809
Changes from 179 commits
19e3035
baf7acd
efe5661
b856795
443bdea
995855d
739ca1f
af7abc8
e6d61d2
a464a05
79f2ebb
a9de009
c9b7e22
5d34a06
121b208
4ab9cc2
13301c1
8f4564e
720caf8
26905c9
7b88806
d9b241c
3f8e10a
3df911f
a1987ef
8cd9cfa
d8af35d
5e50374
50e86b5
c58040f
80f7e69
3f74ffb
a8fd14c
ae50fcc
0d8c7e3
fc99549
dee1b8e
113eb93
2861b7c
390ca93
dab811d
982c418
f56a596
7bdc090
418444f
87c49d1
5735433
64cae08
01d7bdf
603be5d
8fbf1f1
e5b0cd0
50dd0ff
f49b2dd
2ce818d
a0c24af
b087623
e76d474
594dcc9
2a4ef79
436af3d
1e83827
1a96c92
ca0344b
1d7f1f2
cc80caf
7e44ddf
a2b5ef5
fb2810b
89e5fd1
9421ce6
4cf8463
568c1b5
101eeae
1245bbe
cb250c4
14403c3
501d02d
5e32ce0
3a27760
bf3a942
8b32091
3a76b95
972e5a4
3d91e31
f486f63
34fc485
94832de
5926e4b
c15c3bb
a188931
3d35bdb
7c51625
83a2054
41cb9df
0686a13
2ecd87d
277cfaf
3aec7eb
11f6448
ee6c1a6
97ed0d5
f0a719c
9520605
f907699
ea2281c
971e5d0
03c67ee
ba6b1f1
fbc79c9
275a33e
62995e3
1f36baf
9d6c33e
a177c3a
3fa2a1b
62dff53
c036c40
17b3aa2
ce8411a
8b8970c
4634a93
9273268
b5a92db
b225148
2ee4510
a25e91e
409c12b
1d4d9c4
d678d71
acf17e0
79a6079
5fe53db
88d7da9
8064c85
c9df2f9
5307643
f7471f0
7ab754c
03b4a3c
3fcd88f
bf2528b
e2acbc8
5f336c9
81526f6
acc4c71
ad08cdd
2d23dd9
71c4f2d
aea63a8
2aca1f9
ed71785
cbbd3c0
fdb1d9d
1040e2c
a191118
cc1ed73
9ccabad
d33f5ba
8d2ad6c
de6ac15
ce22809
fbe4d46
3180107
d15ad93
451b724
af8dd13
197e215
32e81c4
383d5e7
e65470d
c49c2df
c6f6167
640f5c4
8b59368
cc86c92
8ca7889
d098dcd
0c57f3b
7d6dcfa
779ff27
8c6fd07
1fec75b
9258b30
cac4a47
d821da0
d72f2f7
28fd682
e1c02dc
fa22a68
c3c1c96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,12 +21,14 @@ | |
|
||
import org.elasticsearch.ElasticsearchParseException; | ||
import org.elasticsearch.common.time.DateFormatter; | ||
import org.elasticsearch.common.time.DateFormatters; | ||
import org.elasticsearch.common.xcontent.XContentParser; | ||
import org.elasticsearch.index.mapper.DateFieldMapper; | ||
import org.joda.time.DateTime; | ||
import org.joda.time.DateTimeZone; | ||
|
||
import java.io.IOException; | ||
import java.time.Instant; | ||
import java.time.ZoneOffset; | ||
import java.time.ZonedDateTime; | ||
|
||
public final class WatchStatusDateParser { | ||
|
||
|
@@ -36,14 +38,14 @@ private WatchStatusDateParser() { | |
// Prevent instantiation. | ||
} | ||
|
||
public static DateTime parseDate(String fieldName, XContentParser parser) throws IOException { | ||
public static ZonedDateTime parseDate(String fieldName, XContentParser parser) throws IOException { | ||
XContentParser.Token token = parser.currentToken(); | ||
if (token == XContentParser.Token.VALUE_NUMBER) { | ||
return new DateTime(parser.longValue(), DateTimeZone.UTC); | ||
return Instant.ofEpochMilli(parser.longValue()).atZone(ZoneOffset.UTC); | ||
} | ||
if (token == XContentParser.Token.VALUE_STRING) { | ||
DateTime dateTime = parseDate(parser.text()); | ||
return dateTime.toDateTime(DateTimeZone.UTC); | ||
ZonedDateTime dateTime = parseDate(parser.text()); | ||
return dateTime.withZoneSameInstant(ZoneOffset.UTC); | ||
} | ||
if (token == XContentParser.Token.VALUE_NULL) { | ||
return null; | ||
|
@@ -52,7 +54,7 @@ public static DateTime parseDate(String fieldName, XContentParser parser) throws | |
"to be either a number or a string but found [{}] instead", fieldName, token); | ||
} | ||
|
||
public static DateTime parseDate(String text) { | ||
return FORMATTER.parseJoda(text); | ||
public static ZonedDateTime parseDate(String text) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this method needed or can it be fully replace with DateFormatters.toZonedDateTime(FORMATTER.parse(text))` in the calling method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are two classes that use that. Not sure if we want to inline this, I would need to add this additional field to both places. |
||
return DateFormatters.toZonedDateTime(FORMATTER.parse(text)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,6 +298,8 @@ public void testSingleValuedFieldNormalised_timeZone_CET_DstEnd() throws Excepti | |
* also check for time zone shifts that are not one hour, e.g. | ||
* "Asia/Kathmandu, 1 Jan 1986 - Time Zone Change (IST → NPT), at 00:00:00 clocks were turned forward 00:15 minutes | ||
*/ | ||
// This test fails because we cannot parse negative epoch milli seconds yet... but perhaps we dont have to if we use instants in the | ||
// rangefield method? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did this change sneak in ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should not be here (old comment, not in master). will remove |
||
public void testSingleValuedFieldNormalised_timeZone_AsiaKathmandu() throws Exception { | ||
createIndex(IDX_DST_KATHMANDU); | ||
ZoneId timezone = ZoneId.of("Asia/Kathmandu"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is
withZoneSameInstant
needed here? Maybe add a comment? Is this non UTC somewhere when passed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toDateTime will return a new instance. Still unlikely that there is any comparison of references..
When I worked on this I wasn't aware that we have all DateTImes at UTC.
You recon just assigning the parameter ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to just assign the parameter. We should avoid the appearance of conversions where there are none, or should not be any.
Perhaps a
assert timestamp.getOffset() == ZoneOffset.UTC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will just assign. I like the idea of assert here. Since we have HLRC well covered in tests, this should help spotting if the timezone is badly set