From 7f3711553e28a570a42463969795484c58674ffe Mon Sep 17 00:00:00 2001 From: Noble Paul Date: Mon, 14 Nov 2022 10:07:24 +1100 Subject: [PATCH] Revert "SOLR-16512 : Eliminate noggit JSONWriter.Writable from Solr classes (#1159)" This reverts commit 2dc110850a7efc8066f6d6ddd6c7b427b0d35cf4. SOLR-16530: revrert until perf issues are fixed --- .../snapshots/CollectionSnapshotMetaData.java | 36 ++++--- .../impl/ZkClientClusterStateProvider.java | 4 +- .../org/apache/solr/common/MapWriter.java | 3 - .../solr/common/cloud/ClusterState.java | 27 +++--- .../solr/common/cloud/DocCollection.java | 11 ++- .../apache/solr/common/cloud/DocRouter.java | 9 +- .../org/apache/solr/common/cloud/Replica.java | 22 ++++- .../org/apache/solr/common/cloud/Slice.java | 6 ++ .../apache/solr/common/cloud/ZkNodeProps.java | 8 +- .../apache/solr/common/util/JavaBinCodec.java | 3 - .../solr/common/util/NoggitJSONWriter.java | 97 ------------------- .../apache/solr/common/util/TextWriter.java | 2 - .../org/apache/solr/common/util/Utils.java | 62 +++++++++++- 13 files changed, 141 insertions(+), 149 deletions(-) delete mode 100644 solr/solrj/src/java/org/apache/solr/common/util/NoggitJSONWriter.java diff --git a/solr/core/src/java/org/apache/solr/core/snapshots/CollectionSnapshotMetaData.java b/solr/core/src/java/org/apache/solr/core/snapshots/CollectionSnapshotMetaData.java index fabfb90f52f..483170db598 100644 --- a/solr/core/src/java/org/apache/solr/core/snapshots/CollectionSnapshotMetaData.java +++ b/solr/core/src/java/org/apache/solr/core/snapshots/CollectionSnapshotMetaData.java @@ -16,23 +16,23 @@ */ package org.apache.solr.core.snapshots; -import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Date; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; -import org.apache.solr.common.MapWriter; import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.params.CoreAdminParams; import org.apache.solr.common.util.NamedList; +import org.noggit.JSONWriter; /** This class defines the meta-data about a collection level snapshot */ -public class CollectionSnapshotMetaData implements MapWriter { - public static class CoreSnapshotMetaData implements MapWriter { +public class CollectionSnapshotMetaData implements JSONWriter.Writable { + public static class CoreSnapshotMetaData implements JSONWriter.Writable { private final String coreName; private final String indexDirPath; private final long generationNumber; @@ -90,13 +90,15 @@ public boolean isLeader() { } @Override - public void writeMap(EntryWriter ew) throws IOException { - ew.put(CoreAdminParams.CORE, getCoreName()); - ew.put(SolrSnapshotManager.INDEX_DIR_PATH, getIndexDirPath()); - ew.put(SolrSnapshotManager.GENERATION_NUM, getGenerationNumber()); - ew.put(SolrSnapshotManager.SHARD_ID, getShardId()); - ew.put(SolrSnapshotManager.LEADER, isLeader()); - ew.put(SolrSnapshotManager.FILE_LIST, getFiles()); + public void write(JSONWriter arg0) { + LinkedHashMap info = new LinkedHashMap<>(); + info.put(CoreAdminParams.CORE, getCoreName()); + info.put(SolrSnapshotManager.INDEX_DIR_PATH, getIndexDirPath()); + info.put(SolrSnapshotManager.GENERATION_NUM, getGenerationNumber()); + info.put(SolrSnapshotManager.SHARD_ID, getShardId()); + info.put(SolrSnapshotManager.LEADER, isLeader()); + info.put(SolrSnapshotManager.FILE_LIST, getFiles()); + arg0.write(info); } public NamedList toNamedList() { @@ -227,11 +229,13 @@ public Collection getShards() { } @Override - public void writeMap(EntryWriter ew) throws IOException { - ew.put(CoreAdminParams.NAME, this.name); - ew.put(SolrSnapshotManager.SNAPSHOT_STATUS, this.status.toString()); - ew.put(SolrSnapshotManager.CREATION_DATE, this.getCreationDate().getTime()); - ew.put(SolrSnapshotManager.SNAPSHOT_REPLICAS, this.replicaSnapshots); + public void write(JSONWriter arg0) { + LinkedHashMap result = new LinkedHashMap<>(); + result.put(CoreAdminParams.NAME, this.name); + result.put(SolrSnapshotManager.SNAPSHOT_STATUS, this.status.toString()); + result.put(SolrSnapshotManager.CREATION_DATE, this.getCreationDate().getTime()); + result.put(SolrSnapshotManager.SNAPSHOT_REPLICAS, this.replicaSnapshots); + arg0.write(result); } public NamedList toNamedList() { diff --git a/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java b/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java index 1ff8ba5d253..cfb0911a142 100644 --- a/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java +++ b/solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java @@ -25,7 +25,6 @@ import java.util.Map; import java.util.Set; import org.apache.solr.common.AlreadyClosedException; -import org.apache.solr.common.MapWriter; import org.apache.solr.common.SolrException; import org.apache.solr.common.cloud.ClusterState; import org.apache.solr.common.cloud.DocCollection; @@ -35,6 +34,7 @@ import org.apache.solr.common.cloud.ZooKeeperException; import org.apache.solr.common.util.Utils; import org.apache.zookeeper.KeeperException; +import org.noggit.JSONWriter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -77,7 +77,7 @@ public ZkClientClusterStateProvider(String zkHost) { * * @param bytes a byte array of a Json representation of a mapping from collection name to the * Json representation of a {@link DocCollection} as written by {@link - * ClusterState#writeMap(MapWriter.EntryWriter)}. It can represent one or more collections. + * ClusterState#write(JSONWriter)}. It can represent one or more collections. * @param liveNodes list of live nodes * @param coll collection name * @param zkClient ZK client diff --git a/solr/solrj/src/java/org/apache/solr/common/MapWriter.java b/solr/solrj/src/java/org/apache/solr/common/MapWriter.java index 1fefc98d778..fb05d422a34 100644 --- a/solr/solrj/src/java/org/apache/solr/common/MapWriter.java +++ b/solr/solrj/src/java/org/apache/solr/common/MapWriter.java @@ -129,8 +129,5 @@ default BiConsumer getBiConsumer() { } } - /** A marker interface to denote that this is to be serialized using {@link Object#toString()} */ - interface StringValue {} - MapWriter EMPTY = new MapWriterMap(Collections.emptyMap()); } diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java index 708c0af97fd..a4516f73bf1 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java @@ -18,7 +18,6 @@ import static org.apache.solr.common.util.Utils.STANDARDOBJBUILDER; -import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.Collection; import java.util.Collections; @@ -34,13 +33,13 @@ import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; -import org.apache.solr.common.MapWriter; import org.apache.solr.common.SolrException; import org.apache.solr.common.SolrException.ErrorCode; import org.apache.solr.common.cloud.DocCollection.CollectionStateProps; import org.apache.solr.common.cloud.Replica.ReplicaStateProps; import org.apache.solr.common.util.Utils; import org.noggit.JSONParser; +import org.noggit.JSONWriter; import org.noggit.ObjectBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -51,7 +50,7 @@ * * @lucene.experimental */ -public class ClusterState implements MapWriter { +public class ClusterState implements JSONWriter.Writable { /** Cluster Prop that is http or https. */ public static final String URL_SCHEME = "urlScheme"; @@ -223,8 +222,8 @@ public String toString() { * thus don't call it where that's important * * @param bytes a byte array of a Json representation of a mapping from collection name to the - * Json representation of a {@link DocCollection} as written by {@link - * #writeMap(EntryWriter)}. It can represent one or more collections. + * Json representation of a {@link DocCollection} as written by {@link #write(JSONWriter)}. It + * can represent one or more collections. * @param liveNodes list of live nodes * @return the ClusterState */ @@ -296,8 +295,15 @@ private static DocCollection collectionFromObjects( } @Override - public void writeMap(EntryWriter ew) throws IOException { - collectionStates.forEach(ew.getBiConsumer()); + public void write(JSONWriter jsonWriter) { + LinkedHashMap map = new LinkedHashMap<>(); + for (Entry e : collectionStates.entrySet()) { + if (e.getValue().getClass() == CollectionRef.class) { + DocCollection coll = e.getValue().get(); + map.put(coll.getName(), coll); + } + } + jsonWriter.write(map); } @Override @@ -370,7 +376,7 @@ public void forEachCollection(Consumer consumer) { }); } - public static class CollectionRef implements MapWriter { + public static class CollectionRef { protected final AtomicInteger gets = new AtomicInteger(); private final DocCollection coll; @@ -406,11 +412,6 @@ public boolean isLazilyLoaded() { return false; } - @Override - public void writeMap(EntryWriter ew) throws IOException { - get().writeMap(ew); - } - @Override public String toString() { if (coll != null) { diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java b/solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java index 275e986485e..6d4f6394d45 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java @@ -18,13 +18,13 @@ import static org.apache.solr.common.util.Utils.toJSONString; -import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -32,6 +32,7 @@ import java.util.function.BiConsumer; import java.util.function.BiPredicate; import org.apache.solr.common.cloud.Replica.ReplicaStateProps; +import org.noggit.JSONWriter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -323,9 +324,11 @@ public String toString() { } @Override - public void writeMap(EntryWriter ew) throws IOException { - propMap.forEach(ew.getBiConsumer()); - ew.put(CollectionStateProps.SHARDS, slices); + public void write(JSONWriter jsonWriter) { + LinkedHashMap all = new LinkedHashMap<>(slices.size() + 1); + all.putAll(propMap); + all.put(CollectionStateProps.SHARDS, slices); + jsonWriter.write(all); } public Replica getReplica(String coreNodeName) { diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java b/solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java index 70a4b4b3e5a..af0ebc66f12 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java @@ -25,12 +25,12 @@ import java.util.List; import java.util.Map; import org.apache.solr.cluster.api.HashRange; -import org.apache.solr.common.MapWriter; import org.apache.solr.common.SolrException; import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.cloud.DocCollection.CollectionStateProps; import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.StrUtils; +import org.noggit.JSONWriter; /** * Class to partition int range into n ranges. @@ -89,7 +89,7 @@ public static Map getRouterSpec(ZkNodeProps props) { // Hash ranges can't currently "wrap" - i.e. max must be greater or equal to min. // TODO: ranges may not be all contiguous in the future (either that or we will // need an extra class to model a collection of ranges) - public static class Range implements MapWriter.StringValue, Comparable, HashRange { + public static class Range implements JSONWriter.Writable, Comparable, HashRange { public int min; // inclusive public int max; // inclusive @@ -141,6 +141,11 @@ public boolean equals(Object obj) { return this.min == other.min && this.max == other.max; } + @Override + public void write(JSONWriter writer) { + writer.write(toString()); + } + @Override public int compareTo(Range that) { int mincomp = Integer.compare(this.min, that.min); diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java b/solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java index 87990a5844c..bf1dca1ca96 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java @@ -17,10 +17,12 @@ package org.apache.solr.common.cloud; import static org.apache.solr.common.ConditionalMapWriter.NON_NULL_VAL; +import static org.apache.solr.common.ConditionalMapWriter.dedupeKeyPredicate; import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.Locale; import java.util.Map; @@ -29,6 +31,7 @@ import java.util.function.BiPredicate; import org.apache.solr.common.MapWriter; import org.apache.solr.common.util.Utils; +import org.noggit.JSONWriter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -366,7 +369,7 @@ public Object clone() { @Override public void writeMap(MapWriter.EntryWriter ew) throws IOException { - _allPropsWriter().writeMap(ew); + ew.put(name, _allPropsWriter()); } private static final Map STATES = new HashMap<>(); @@ -383,15 +386,16 @@ public static State getState(String shortName) { } private MapWriter _allPropsWriter() { - BiPredicate p = - ((BiPredicate) (k, o) -> !propMap.containsKey(k.toString())) - .and(NON_NULL_VAL); + BiPredicate p = dedupeKeyPredicate(new HashSet<>()).and(NON_NULL_VAL); return writer -> { // XXX this is why this class should be immutable - it's a mess !!! // propMap takes precedence because it's mutable and we can't control its // contents, so a third party may override some declared fields - propMap.forEach(writer.getBiConsumer()); + for (Map.Entry e : propMap.entrySet()) { + writer.put(e.getKey(), e.getValue(), p); + } + writer .put(ReplicaStateProps.CORE_NAME, core, p) .put(ReplicaStateProps.SHARD_ID, shard, p) @@ -402,6 +406,14 @@ private MapWriter _allPropsWriter() { }; } + @Override + public void write(JSONWriter jsonWriter) { + Map map = new LinkedHashMap<>(); + // this serializes also our declared properties + _allPropsWriter().toMap(map); + jsonWriter.write(map); + } + @Override public String toString() { return name diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java b/solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java index c8f0e7bbbfa..5414942b480 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java @@ -32,6 +32,7 @@ import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.solr.common.cloud.Replica.Type; +import org.noggit.JSONWriter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -306,6 +307,11 @@ public String toString() { return name + ':' + toJSONString(propMap); } + @Override + public void write(JSONWriter jsonWriter) { + jsonWriter.write(propMap); + } + /** JSON properties related to a slice's state. */ public interface SliceStateProps { diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java index b187e475fe8..0e6dcef7108 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java @@ -28,9 +28,10 @@ import org.apache.solr.common.MapWriter; import org.apache.solr.common.util.JavaBinCodec; import org.apache.solr.common.util.Utils; +import org.noggit.JSONWriter; /** ZkNodeProps contains generic immutable properties. */ -public class ZkNodeProps implements MapWriter { +public class ZkNodeProps implements JSONWriter.Writable, MapWriter { protected final Map propMap; @@ -104,6 +105,11 @@ public static ZkNodeProps load(byte[] bytes) { return new ZkNodeProps(props); } + @Override + public void write(JSONWriter jsonWriter) { + jsonWriter.write(propMap); + } + /** Get a string property value. */ public String getStr(String key) { return getStr(key, null); diff --git a/solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java b/solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java index c4a44090d1e..abe9c9d8619 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java @@ -1102,9 +1102,6 @@ public boolean writePrimitive(Object val) throws IOException { } else if (val instanceof LongAccumulator) { daos.writeLong(((LongAccumulator) val).longValue()); return true; - } else if (val instanceof MapWriter.StringValue) { - writeStr(val.toString()); - return true; } return false; } diff --git a/solr/solrj/src/java/org/apache/solr/common/util/NoggitJSONWriter.java b/solr/solrj/src/java/org/apache/solr/common/util/NoggitJSONWriter.java deleted file mode 100644 index d31e598604c..00000000000 --- a/solr/solrj/src/java/org/apache/solr/common/util/NoggitJSONWriter.java +++ /dev/null @@ -1,97 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.solr.common.util; - -import java.io.IOException; -import org.apache.solr.common.IteratorWriter; -import org.apache.solr.common.MapWriter; -import org.noggit.CharArr; -import org.noggit.JSONWriter; - -/** Serialize JSON using noggit */ -public class NoggitJSONWriter extends JSONWriter { - - public NoggitJSONWriter(CharArr out, int indentSize) { - super(out, indentSize); - } - - @Override - public void write(Object o) { - if (o instanceof MapWriter) { - MapWriter mapWriter = (MapWriter) o; - startObject(); - try { - mapWriter.writeMap(entryWriter()); - } catch (IOException e) { - throw new RuntimeException(e); - } - endObject(); - } else if (o instanceof IteratorWriter) { - IteratorWriter iteratorWriter = (IteratorWriter) o; - startArray(); - try { - iteratorWriter.writeIter(itemWriter()); - } catch (IOException e) { - throw new RuntimeException("this should never happen", e); - } - endArray(); - } else if (o instanceof MapWriter.StringValue) { - super.write(o.toString()); - } else { - super.write(o); - } - } - - private IteratorWriter.ItemWriter itemWriter() { - return new IteratorWriter.ItemWriter() { - private boolean first; - - @Override - public IteratorWriter.ItemWriter add(Object o) { - if (first) { - first = false; - } else { - writeValueSeparator(); - } - indent(); - write(o); - return this; - } - }; - } - - private MapWriter.EntryWriter entryWriter() { - return new MapWriter.EntryWriter() { - private boolean first; - - @Override - public MapWriter.EntryWriter put(CharSequence k, Object v) { - if (first) { - first = false; - } else { - writeValueSeparator(); - } - indent(); - writeString(k.toString()); - writeNameSeparator(); - write(v); - return this; - } - }; - } -} diff --git a/solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java b/solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java index 66b6bcff23e..3ec8df2168b 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java @@ -105,8 +105,6 @@ default void writeVal(String name, Object val, boolean raw) throws IOException { } else { writeStr(name, val.toString(), true); } - } else if (val instanceof MapWriter.StringValue) { - writeStr(name, val.toString(), true); } else { // default... for debugging only. Would be nice to "assert false" ? writeStr(name, val.getClass().getName() + ':' + val.toString(), true); diff --git a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java index 8c062b00680..1a3a737fd2a 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java @@ -81,6 +81,7 @@ import org.apache.solr.common.params.CommonParams; import org.noggit.CharArr; import org.noggit.JSONParser; +import org.noggit.JSONWriter; import org.noggit.ObjectBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -213,6 +214,65 @@ public static Writer writeJson(Object o, Writer writer, boolean indent) throws I return writer; } + private static class MapWriterJSONWriter extends JSONWriter { + + public MapWriterJSONWriter(CharArr out, int indentSize) { + super(out, indentSize); + } + + @Override + public void handleUnknownClass(Object o) { + // avoid materializing MapWriter / IteratorWriter to Map / List + // instead serialize them directly + if (o instanceof MapWriter) { + MapWriter mapWriter = (MapWriter) o; + startObject(); + final boolean[] first = new boolean[1]; + first[0] = true; + int sz = mapWriter._size(); + mapWriter._forEachEntry( + (k, v) -> { + if (first[0]) { + first[0] = false; + } else { + writeValueSeparator(); + } + if (sz > 1) indent(); + writeString(k.toString()); + writeNameSeparator(); + write(v); + }); + endObject(); + } else if (o instanceof IteratorWriter) { + IteratorWriter iteratorWriter = (IteratorWriter) o; + startArray(); + final boolean[] first = new boolean[1]; + first[0] = true; + try { + iteratorWriter.writeIter( + new IteratorWriter.ItemWriter() { + @Override + public IteratorWriter.ItemWriter add(Object o) throws IOException { + if (first[0]) { + first[0] = false; + } else { + writeValueSeparator(); + } + indent(); + write(o); + return this; + } + }); + } catch (IOException e) { + throw new RuntimeException("this should never happen", e); + } + endArray(); + } else { + super.handleUnknownClass(o); + } + } + } + public static byte[] toJSON(Object o) { if (o == null) return new byte[0]; CharArr out = new CharArr(); @@ -223,7 +283,7 @@ public static byte[] toJSON(Object o) { // o = ((IteratorWriter) o).toList(new ArrayList<>()); // } // } - new NoggitJSONWriter(out, 2).write(o); // indentation by default + new MapWriterJSONWriter(out, 2).write(o); // indentation by default return toUTF8(out); }