[Remove] Joda from Serialization (opensearch-project#9350)

In Legacy 7.0 joda time was removed in favor of java time to support nanoseconds properly. This commit removes joda from the serialization including ignoring the joda flag sent over the wire for DocValueFormat. Signed-off-by: Nicholas Walter Knize <[email protected]>
austintlee · Aug 25, 2023 · e92d7c1 · e92d7c1
1 parent e154a23
commit e92d7c1
Show file tree

Hide file tree

Showing 11 changed files with 55 additions and 162 deletions.
diff --git a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java
@@ -722,6 +722,8 @@ public Object readGenericValue() throws IOException {
                 return readByte();
             case 12:
                 return readDate();
+            case 13:
+                return readZonedDateTime();
             case 14:
                 return readBytesReference();
             case 15:

diff --git a/server/src/main/java/org/opensearch/common/io/stream/Streamables.java b/server/src/main/java/org/opensearch/common/io/stream/Streamables.java
@@ -9,16 +9,9 @@
 package org.opensearch.common.io.stream;
 
 import org.opensearch.common.geo.GeoPoint;
-import org.opensearch.common.time.DateUtils;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.common.io.stream.Writeable.WriteableRegistry;
-import org.joda.time.DateTimeZone;
-import org.joda.time.ReadableInstant;
-
-import java.time.Instant;
-import java.time.ZoneId;
-import java.time.ZonedDateTime;
 
 /**
  * This utility class registers generic types for streaming over the wire using
@@ -47,14 +40,6 @@ public static void registerStreamables() {
      * Registers writers by class type
      */
     private static void registerWriters() {
-        /** {@link ReadableInstant} */
-        WriteableRegistry.registerWriter(ReadableInstant.class, (o, v) -> {
-            o.writeByte((byte) 13);
-            final ReadableInstant instant = (ReadableInstant) v;
-            o.writeString(instant.getZone().getID());
-            o.writeLong(instant.getMillis());
-        });
-        WriteableRegistry.registerClassAlias(ReadableInstant.class, ReadableInstant.class);
         /** {@link GeoPoint} */
         WriteableRegistry.registerWriter(GeoPoint.class, (o, v) -> {
             o.writeByte((byte) 22);
@@ -68,12 +53,6 @@ private static void registerWriters() {
      * NOTE: see {@code StreamOutput#WRITERS} for all registered ordinals
      */
     private static void registerReaders() {
-        /** {@link ZonedDateTime} */
-        WriteableRegistry.registerReader(Byte.valueOf((byte) 13), (i) -> {
-            final ZoneId zoneId = DateUtils.dateTimeZoneToZoneId(DateTimeZone.forID(i.readString()));
-            long millis = i.readLong();
-            return ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), zoneId);
-        });
         /** {@link GeoPoint} */
         WriteableRegistry.registerReader(Byte.valueOf((byte) 22), GeoPoint::new);
     }

diff --git a/server/src/main/java/org/opensearch/common/xcontent/XContentOpenSearchExtension.java b/server/src/main/java/org/opensearch/common/xcontent/XContentOpenSearchExtension.java
@@ -39,18 +39,10 @@
 import org.opensearch.core.common.unit.ByteSizeValue;
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.core.xcontent.XContentBuilderExtension;
-import org.joda.time.DateTime;
-import org.joda.time.DateTimeZone;
-import org.joda.time.Instant;
-import org.joda.time.MutableDateTime;
-import org.joda.time.ReadableInstant;
-import org.joda.time.format.DateTimeFormatter;
-import org.joda.time.format.ISODateTimeFormat;
-import org.joda.time.tz.CachedDateTimeZone;
-import org.joda.time.tz.FixedDateTimeZone;
 
 import java.time.DayOfWeek;
 import java.time.Duration;
+import java.time.Instant;
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
@@ -79,7 +71,6 @@
  */
 public class XContentOpenSearchExtension implements XContentBuilderExtension {
 
-    public static final DateTimeFormatter DEFAULT_DATE_PRINTER = ISODateTimeFormat.dateTime().withZone(DateTimeZone.UTC);
     public static final DateFormatter DEFAULT_FORMATTER = DateFormatter.forPattern("strict_date_optional_time_nanos");
     public static final DateFormatter LOCAL_TIME_FORMATTER = DateFormatter.forPattern("HH:mm:ss.SSS");
     public static final DateFormatter OFFSET_TIME_FORMATTER = DateFormatter.forPattern("HH:mm:ss.SSSZZZZZ");
@@ -90,11 +81,6 @@ public Map<Class<?>, XContentBuilder.Writer> getXContentWriters() {
 
         // Fully-qualified here to reduce ambiguity around our (OpenSearch') Version class
         writers.put(org.apache.lucene.util.Version.class, (b, v) -> b.value(Objects.toString(v)));
-        writers.put(DateTimeZone.class, (b, v) -> b.value(Objects.toString(v)));
-        writers.put(CachedDateTimeZone.class, (b, v) -> b.value(Objects.toString(v)));
-        writers.put(FixedDateTimeZone.class, (b, v) -> b.value(Objects.toString(v)));
-        writers.put(MutableDateTime.class, XContentBuilder::timeValue);
-        writers.put(DateTime.class, XContentBuilder::timeValue);
         writers.put(TimeValue.class, (b, v) -> b.value(v.toString()));
         writers.put(ZonedDateTime.class, XContentBuilder::timeValue);
         writers.put(OffsetDateTime.class, XContentBuilder::timeValue);
@@ -141,14 +127,11 @@ public Map<Class<?>, XContentBuilder.HumanReadableTransformer> getXContentHumanR
     @Override
     public Map<Class<?>, Function<Object, Object>> getDateTransformers() {
         Map<Class<?>, Function<Object, Object>> transformers = new HashMap<>();
-        transformers.put(Date.class, d -> DEFAULT_DATE_PRINTER.print(((Date) d).getTime()));
-        transformers.put(DateTime.class, d -> DEFAULT_DATE_PRINTER.print((DateTime) d));
-        transformers.put(MutableDateTime.class, d -> DEFAULT_DATE_PRINTER.print((MutableDateTime) d));
-        transformers.put(ReadableInstant.class, d -> DEFAULT_DATE_PRINTER.print((ReadableInstant) d));
-        transformers.put(Long.class, d -> DEFAULT_DATE_PRINTER.print((long) d));
-        transformers.put(Calendar.class, d -> DEFAULT_DATE_PRINTER.print(((Calendar) d).getTimeInMillis()));
-        transformers.put(GregorianCalendar.class, d -> DEFAULT_DATE_PRINTER.print(((Calendar) d).getTimeInMillis()));
-        transformers.put(Instant.class, d -> DEFAULT_DATE_PRINTER.print((Instant) d));
+        transformers.put(Date.class, d -> DEFAULT_FORMATTER.format(((Date) d).toInstant()));
+        transformers.put(Long.class, d -> DEFAULT_FORMATTER.format(Instant.ofEpochMilli((long) d)));
+        transformers.put(Calendar.class, d -> DEFAULT_FORMATTER.format(((Calendar) d).toInstant()));
+        transformers.put(GregorianCalendar.class, d -> DEFAULT_FORMATTER.format(((Calendar) d).toInstant()));
+        transformers.put(Instant.class, d -> DEFAULT_FORMATTER.format((Instant) d));
         transformers.put(ZonedDateTime.class, d -> DEFAULT_FORMATTER.format((ZonedDateTime) d));
         transformers.put(OffsetDateTime.class, d -> DEFAULT_FORMATTER.format((OffsetDateTime) d));
         transformers.put(OffsetTime.class, d -> OFFSET_TIME_FORMATTER.format((OffsetTime) d));

diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestCatRecoveryAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestCatRecoveryAction.java
@@ -48,6 +48,7 @@
 import org.opensearch.rest.RestResponse;
 import org.opensearch.rest.action.RestResponseListener;
 
+import java.time.Instant;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Locale;
@@ -170,9 +171,9 @@ public int compare(RecoveryState o1, RecoveryState o2) {
                 t.startRow();
                 t.addCell(index);
                 t.addCell(state.getShardId().id());
-                t.addCell(XContentOpenSearchExtension.DEFAULT_DATE_PRINTER.print(state.getTimer().startTime()));
+                t.addCell(XContentOpenSearchExtension.DEFAULT_FORMATTER.format(Instant.ofEpochMilli(state.getTimer().startTime())));
                 t.addCell(state.getTimer().startTime());
-                t.addCell(XContentOpenSearchExtension.DEFAULT_DATE_PRINTER.print(state.getTimer().stopTime()));
+                t.addCell(XContentOpenSearchExtension.DEFAULT_FORMATTER.format(Instant.ofEpochMilli(state.getTimer().stopTime())));
                 t.addCell(state.getTimer().stopTime());
                 t.addCell(new TimeValue(state.getTimer().time()));
                 t.addCell(state.getRecoverySource().getType().toString().toLowerCase(Locale.ROOT));

diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestCatSegmentReplicationAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestCatSegmentReplicationAction.java
@@ -27,6 +27,7 @@
 import org.opensearch.rest.RestResponse;
 import org.opensearch.rest.action.RestResponseListener;
 
+import java.time.Instant;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -180,8 +181,8 @@ public Table buildSegmentReplicationTable(RestRequest request, SegmentReplicatio
                         t.addCell(String.format(Locale.ROOT, "%1.1f%%", state.getIndex().recoveredFilesPercent()));
                         t.addCell(state.getIndex().recoveredBytes());
                         t.addCell(String.format(Locale.ROOT, "%1.1f%%", state.getIndex().recoveredBytesPercent()));
-                        t.addCell(XContentOpenSearchExtension.DEFAULT_DATE_PRINTER.print(state.getTimer().startTime()));
-                        t.addCell(XContentOpenSearchExtension.DEFAULT_DATE_PRINTER.print(state.getTimer().stopTime()));
+                        t.addCell(XContentOpenSearchExtension.DEFAULT_FORMATTER.format(Instant.ofEpochMilli(state.getTimer().startTime())));
+                        t.addCell(XContentOpenSearchExtension.DEFAULT_FORMATTER.format(Instant.ofEpochMilli(state.getTimer().stopTime())));
                         t.addCell(state.getIndex().totalRecoverFiles());
                         t.addCell(state.getIndex().totalFileCount());
                         t.addCell(new ByteSizeValue(state.getIndex().totalRecoverBytes()));

diff --git a/server/src/main/java/org/opensearch/search/DocValueFormat.java b/server/src/main/java/org/opensearch/search/DocValueFormat.java
@@ -34,9 +34,8 @@
 
 import org.apache.lucene.document.InetAddressPoint;
 import org.apache.lucene.util.BytesRef;
+import org.opensearch.Version;
 import org.opensearch.common.Numbers;
-import org.opensearch.common.joda.Joda;
-import org.opensearch.common.joda.JodaDateFormatter;
 import org.opensearch.common.network.InetAddresses;
 import org.opensearch.common.network.NetworkAddress;
 import org.opensearch.common.time.DateFormatter;
@@ -244,13 +243,14 @@ public DateTime(DateFormatter formatter, ZoneId timeZone, DateFieldMapper.Resolu
         }
 
         public DateTime(StreamInput in) throws IOException {
-            String datePattern = in.readString();
+            this.formatter = DateFormatter.forPattern(in.readString());
+            this.parser = formatter.toDateMathParser();
             String zoneId = in.readString();
             this.timeZone = ZoneId.of(zoneId);
             this.resolution = DateFieldMapper.Resolution.ofOrdinal(in.readVInt());
-            final boolean isJoda = in.readBoolean();
-            this.formatter = isJoda ? Joda.forPattern(datePattern) : DateFormatter.forPattern(datePattern);
-            this.parser = formatter.toDateMathParser();
+            if (in.getVersion().before(Version.V_3_0_0)) {
+                in.readBoolean(); // ignore deprecated joda
+            }
         }
 
         @Override
@@ -263,8 +263,9 @@ public void writeTo(StreamOutput out) throws IOException {
             out.writeString(formatter.pattern());
             out.writeString(timeZone.getId());
             out.writeVInt(resolution.ordinal());
-            // in order not to loose information if the formatter is a joda we send a flag
-            out.writeBoolean(formatter instanceof JodaDateFormatter);// todo pg consider refactor to isJoda method..
+            if (out.getVersion().before(Version.V_3_0_0)) {
+                out.writeBoolean(false); // ignore deprecated joda flag
+            }
         }
 
         public DateMathParser getDateMathParser() {

diff --git a/server/src/test/java/org/opensearch/cluster/metadata/IndexGraveyardTests.java b/server/src/test/java/org/opensearch/cluster/metadata/IndexGraveyardTests.java
@@ -36,7 +36,6 @@
 import org.opensearch.common.io.stream.BytesStreamOutput;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.xcontent.XContentOpenSearchExtension;
-import org.opensearch.common.xcontent.json.JsonXContent;
 import org.opensearch.core.common.Strings;
 import org.opensearch.core.common.bytes.BytesReference;
 import org.opensearch.core.index.Index;
@@ -47,6 +46,7 @@
 import org.opensearch.test.OpenSearchTestCase;
 
 import java.io.IOException;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -80,7 +80,7 @@ public void testSerialization() throws IOException {
 
     public void testXContent() throws IOException {
         final IndexGraveyard graveyard = createRandom();
-        final XContentBuilder builder = JsonXContent.contentBuilder();
+        final XContentBuilder builder = MediaTypeRegistry.JSON.contentBuilder();
         builder.startObject();
         graveyard.toXContent(builder, ToXContent.EMPTY_PARAMS);
         builder.endObject();
@@ -89,11 +89,13 @@ public void testXContent() throws IOException {
             assertThat(
                 Strings.toString(MediaTypeRegistry.JSON, graveyard, false, true),
                 containsString(
-                    XContentOpenSearchExtension.DEFAULT_DATE_PRINTER.print(graveyard.getTombstones().get(0).getDeleteDateInMillis())
+                    XContentOpenSearchExtension.DEFAULT_FORMATTER.format(
+                        Instant.ofEpochMilli(graveyard.getTombstones().get(0).getDeleteDateInMillis())
+                    )
                 )
             );
         }
-        XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder));
+        XContentParser parser = createParser(MediaTypeRegistry.JSON.xContent(), BytesReference.bytes(builder));
         parser.nextToken(); // the beginning of the parser
         assertThat(IndexGraveyard.fromXContent(parser), equalTo(graveyard));
     }

diff --git a/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java b/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java
@@ -36,7 +36,6 @@
 import org.apache.lucene.util.BytesRef;
 import org.apache.lucene.util.Constants;
 import org.opensearch.common.geo.GeoPoint;
-import org.opensearch.common.joda.Joda;
 import org.opensearch.common.lucene.BytesRefs;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.common.util.PageCacheRecycler;
@@ -49,11 +48,10 @@
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.common.io.stream.Writeable;
 import org.opensearch.test.OpenSearchTestCase;
-import org.joda.time.DateTime;
-import org.joda.time.DateTimeZone;
 
 import java.io.EOFException;
 import java.io.IOException;
+import java.time.Instant;
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.util.ArrayList;
@@ -327,10 +325,9 @@ public void testSimpleStreams() throws Exception {
         out.writeOptionalBytesReference(new BytesArray("test"));
         out.writeOptionalDouble(null);
         out.writeOptionalDouble(1.2);
-        Joda.writeTimeZone(out, DateTimeZone.forID("CET"));
-        Joda.writeOptionalTimeZone(out, DateTimeZone.getDefault());
-        Joda.writeOptionalTimeZone(out, null);
-        out.writeGenericValue(new DateTime(123456, DateTimeZone.forID("America/Los_Angeles")));
+        out.writeZoneId(ZoneId.of("CET"));
+        out.writeOptionalZoneId(ZoneId.systemDefault());
+        out.writeGenericValue(ZonedDateTime.ofInstant(Instant.ofEpochMilli(123456), ZoneId.of("America/Los_Angeles")));
         final byte[] bytes = BytesReference.toBytes(out.bytes());
         StreamInput in = StreamInput.wrap(BytesReference.toBytes(out.bytes()));
         assertEquals(in.available(), bytes.length);
@@ -360,9 +357,8 @@ public void testSimpleStreams() throws Exception {
         assertThat(in.readOptionalBytesReference(), equalTo(new BytesArray("test")));
         assertNull(in.readOptionalDouble());
         assertThat(in.readOptionalDouble(), closeTo(1.2, 0.0001));
-        assertEquals(DateTimeZone.forID("CET"), Joda.readTimeZone(in));
-        assertEquals(DateTimeZone.getDefault(), Joda.readOptionalTimeZone(in));
-        assertNull(Joda.readOptionalTimeZone(in));
+        assertEquals(ZoneId.of("CET"), in.readZoneId());
+        assertEquals(ZoneId.systemDefault(), in.readOptionalZoneId());
         Object dt = in.readGenericValue();
         assertThat(dt, instanceOf(ZonedDateTime.class));
         ZonedDateTime zdt = (ZonedDateTime) dt;