-
Notifications
You must be signed in to change notification settings - Fork 831
SOLR-17600 (Part 1/4): Migrate V2 API Payloads from MapSerializable #4463
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ | |
| import org.apache.solr.client.solrj.response.SimpleSolrResponse; | ||
| import org.apache.solr.cloud.ZkController; | ||
| import org.apache.solr.cloud.ZkSolrResourceLoader; | ||
| import org.apache.solr.common.MapSerializable; | ||
| import org.apache.solr.common.MapWriter; | ||
| import org.apache.solr.common.SolrErrorWrappingException; | ||
| import org.apache.solr.common.SolrException; | ||
| import org.apache.solr.common.cloud.ClusterState; | ||
|
|
@@ -75,6 +75,7 @@ | |
| import org.apache.solr.common.util.EnvUtils; | ||
| import org.apache.solr.common.util.ExecutorUtil; | ||
| import org.apache.solr.common.util.NamedList; | ||
| import org.apache.solr.common.util.SimpleOrderedMap; | ||
| import org.apache.solr.common.util.SolrNamedThreadFactory; | ||
| import org.apache.solr.common.util.StrUtils; | ||
| import org.apache.solr.common.util.Utils; | ||
|
|
@@ -204,7 +205,7 @@ private void handleGET() { | |
| Map<String, Object> m = new LinkedHashMap<>(); | ||
| m.put(ZNODEVER, params.getZnodeVersion()); | ||
| if (p != null) { | ||
| m.put(RequestParams.NAME, Map.of(parts.get(2), p.toMap(new LinkedHashMap<>()))); | ||
| m.put(RequestParams.NAME, Map.of(parts.get(2), p)); | ||
| } | ||
| resp.add(SolrQueryResponse.NAME, m); | ||
| } else { | ||
|
|
@@ -283,16 +284,16 @@ private void handleGET() { | |
| Map pluginNameVsPluginInfo = (Map) val.get(parts.get(1)); | ||
| if (pluginNameVsPluginInfo != null) { | ||
| Object o = | ||
| pluginNameVsPluginInfo instanceof MapSerializable | ||
| pluginNameVsPluginInfo instanceof MapWriter | ||
| ? pluginNameVsPluginInfo | ||
| : pluginNameVsPluginInfo.get(componentName); | ||
| Map<String, Object> pluginInfo = | ||
| o instanceof MapSerializable | ||
| ? ((MapSerializable) o).toMap(new LinkedHashMap<>()) | ||
| o instanceof MapWriter | ||
| ? new SimpleOrderedMap<>((MapWriter) o) | ||
|
Contributor
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. SOM is okay for response
Contributor
Author
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. Keeping SOM here. The resulting
Contributor
Author
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.
|
||
| : (Map<String, Object>) o; | ||
| val.put( | ||
| parts.get(1), | ||
| pluginNameVsPluginInfo instanceof PluginInfo | ||
| pluginNameVsPluginInfo instanceof MapWriter | ||
| ? pluginInfo | ||
| : Map.of(componentName, pluginInfo)); | ||
| if (req.getParams().getBool("meta", false)) { | ||
|
|
@@ -306,8 +307,10 @@ private void handleGET() { | |
| if (infos == null || infos.isEmpty()) continue; | ||
| infos.forEach( | ||
| (s, mapWriter) -> { | ||
| if (s.equals(pluginInfo.get("class"))) { | ||
| (pluginInfo).put("_packageinfo_", mapWriter); | ||
| Map<String, Object> componentInfo = | ||
| getComponentInfo(pluginInfo, componentName); | ||
| if (s.equals(componentInfo.get("class"))) { | ||
| (componentInfo).put("_packageinfo_", mapWriter); | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -320,6 +323,16 @@ private void handleGET() { | |
| } | ||
| } | ||
|
|
||
| @SuppressWarnings({"unchecked"}) | ||
| private Map<String, Object> getComponentInfo( | ||
| Map<String, Object> pluginInfo, String componentName) { | ||
| if (pluginInfo.containsKey("class")) { | ||
| return pluginInfo; | ||
| } | ||
| return (Map<String, Object>) | ||
| pluginInfo.computeIfAbsent(componentName, k -> new LinkedHashMap<>()); | ||
| } | ||
|
|
||
| private Map<String, Object> getConfigDetails(String componentType, SolrQueryRequest req) { | ||
| String componentName = componentType == null ? null : req.getParams().get("componentName"); | ||
| boolean showParams = req.getParams().getBool("expandParams", false); | ||
|
|
@@ -510,9 +523,7 @@ private void handleParams(ArrayList<CommandOperation> ops, RequestParams params) | |
| ZkController.touchConfDir(zkLoader); | ||
| } else { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug( | ||
| "persisting params data : {}", | ||
| Utils.toJSONString(params.toMap(new LinkedHashMap<>()))); | ||
| log.debug("persisting params data : {}", Utils.toJSONString(params)); | ||
| } | ||
| int latestVersion = | ||
| ZkController.persistConfigResourceToZooKeeper( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,7 @@ | |
| import org.apache.lucene.util.SuppressForbidden; | ||
| import org.apache.lucene.util.UnicodeUtil; | ||
| import org.apache.solr.common.MapWriter; | ||
| import org.apache.solr.common.util.SimpleOrderedMap; | ||
| import org.apache.solr.common.util.Utils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
@@ -277,12 +278,10 @@ private void convert(Map<String, Object> result) { | |
| for (Map.Entry<String, Object> entry : result.entrySet()) { | ||
| Object value = entry.getValue(); | ||
| if (value instanceof ItemPriorityQueue queue) { | ||
| Map<String, Object> map = new LinkedHashMap<>(); | ||
| queue.toMap(map); | ||
| Map<String, Object> map = new SimpleOrderedMap<>(queue); | ||
| entry.setValue(map); | ||
| } else if (value instanceof MapWriterSummaryStatistics stats) { | ||
| Map<String, Object> map = new LinkedHashMap<>(); | ||
| stats.toMap(map); | ||
| Map<String, Object> map = new SimpleOrderedMap<>(stats); | ||
|
Comment on lines
+281
to
+284
Contributor
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. SOM is good here; this is a response-structure
Contributor
Author
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. Agreed — keeping SOM.
Contributor
Author
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.
|
||
| entry.setValue(map); | ||
| } else if (value instanceof AtomicLong) { | ||
| entry.setValue(((AtomicLong) value).longValue()); | ||
|
|
||
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.
The code here seems mostly populating a response. For such cases, I question if you need to convert existing MapWriter things to SimpleOrderedMap (e.g. by using SOM's constructor). Meaning, simply pass the MapWriter as-is. I believe the response writers will all check the value to be of type MapWriter and then do the right thing. I mentioned this point in Part 2 previously.
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.
Applied at line 208 where the value isn't mutated. The remaining SOM conversions (lines 292, 339, 372) all feed code paths that subsequently call
computeIfAbsent/put/setValueon the map, so they need a mutableMapand the conversion stays.Note: 339 and 372 were also broken in the originally pushed code (tried to wrap a
MapSerializableinSimpleOrderedMap, which only has aMapWriterconstructor — premature change that assumed Part 2's migration). I reverted them to.toMap(new LinkedHashMap<>())so Part 1 compiles standalone; the SOM-wrap will land naturally in Part 2 onceSolrConfig/PluginInfobecomeMapWriter.