SOLR-17600 (Part 2/4): Migrate Core Configuration classes from MapSerializable#4464
SOLR-17600 (Part 2/4): Migrate Core Configuration classes from MapSerializable#4464isaric wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates several Solr config/metadata classes and related call sites from the legacy MapSerializable#toMap(...) pattern to the newer MapWriter#writeMap(...) API, updating tests and various admin/handler code paths accordingly.
Changes:
- Replace
MapSerializableimplementations withMapWriter(writeMap) across core config-related classes. - Update call sites to materialize map-like structures via
SimpleOrderedMaporUtils.convertToMap(...)instead oftoMap(new HashMap<>()). - Adjust tests/assertions to reflect the new
MapWriter/SimpleOrderedMapbased conversions.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java | Update test to use SimpleOrderedMap and MapWriter assertions. |
| solr/core/src/test/org/apache/solr/handler/designer/TestSchemaDesignerAPI.java | Use SimpleOrderedMap instead of SolrParams#toMap(...). |
| solr/core/src/test/org/apache/solr/core/TestConfig.java | Switch default mapping checks to SimpleOrderedMap. |
| solr/core/src/test/org/apache/solr/core/CacheConfigTest.java | Convert CacheConfig to map via SimpleOrderedMap. |
| solr/core/src/test/org/apache/solr/api/NodeConfigClusterPluginsSourceTest.java | Convert MapWriter config to map via SimpleOrderedMap. |
| solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java | Migrate SolrIndexConfig from MapSerializable to MapWriter. |
| solr/core/src/java/org/apache/solr/update/IndexFingerprint.java | Migrate IndexFingerprint to MapWriter; update toString() materialization. |
| solr/core/src/java/org/apache/solr/search/CacheConfig.java | Migrate CacheConfig to MapWriter. |
| solr/core/src/java/org/apache/solr/schema/IndexSchema.java | Migrate SchemaProps to MapWriter; adjust property retrieval to SimpleOrderedMap. |
| solr/core/src/java/org/apache/solr/handler/export/ExportWriterStream.java | Materialize nested MapWriter values using SimpleOrderedMap. |
| solr/core/src/java/org/apache/solr/handler/designer/ManagedSchemaDiff.java | Convert SimpleOrderedMap to Map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/SplitShardAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/SplitCoreAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/RequestSyncShardAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/RequestCoreRecoveryAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/RequestBufferUpdatesAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/RequestApplyCoreUpdatesAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/RejoinLeaderElectionAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/RebalanceLeadersAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/PrepareCoreRecoveryAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/OverseerOperationAPI.java | Convert request payload using SimpleOrderedMap instead of toMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/MoveReplicaAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/ModifyCollectionAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/MigrateDocsAPI.java | Convert request payload to v1 param map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/InstallShardData.java | Convert message payload into ZkNodeProps via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/api/CreateCore.java | Convert v1 params to request body via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java | Convert DocCollection to map via Utils.convertToMap(...). |
| solr/core/src/java/org/apache/solr/handler/admin/IndexSizeEstimator.java | Materialize MapWriter values via SimpleOrderedMap. |
| solr/core/src/java/org/apache/solr/handler/admin/BaseHandlerApiSupport.java | Switch anonymous params object to MapWriter#writeMap. |
| solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java | Update config introspection logic to use MapWriter/SimpleOrderedMap. |
| solr/core/src/java/org/apache/solr/handler/ClusterAPI.java | Replace payload toMap(...) with SimpleOrderedMap conversion. |
| solr/core/src/java/org/apache/solr/filestore/ClusterFileStore.java | Copy metadata into unknown properties via SimpleOrderedMap. |
| solr/core/src/java/org/apache/solr/core/SolrConfig.java | Migrate SolrConfig and inner config holders to MapWriter. |
| solr/core/src/java/org/apache/solr/core/RequestParams.java | Migrate RequestParams and ParamSet to MapWriter. |
| solr/core/src/java/org/apache/solr/core/PluginInfo.java | Migrate PluginInfo to MapWriter. |
| solr/core/src/java/org/apache/solr/core/ConfigOverlay.java | Migrate ConfigOverlay to MapWriter. |
| solr/core/src/java/org/apache/solr/api/NodeConfigClusterPluginsSource.java | Use NamedList#asMap(...) for plugin initArgs config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new NamedList<>(attributes).writeMap(ew); | ||
| if (initArgs != null) { | ||
| new SimpleOrderedMap<>(initArgs).writeMap(ew); | ||
| } | ||
| if (children == null || children.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| for (PluginInfo child : children) { | ||
| child.writeMap(ew); | ||
| } |
| && !((List) o).isEmpty() | ||
| && ((List) o).get(0) instanceof String) { |
d94547f to
37d86fe
Compare
37d86fe to
d0fa05e
Compare
| } else if (old instanceof List) { | ||
| ((List<Object>) old).add(child); |
There was a problem hiding this comment.
as it was before, can use Java 17 instanceof auto cast to 'list" here
| ew.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM, luceneMatchVersion.toString()); | ||
| } | ||
|
|
||
| ew.put("updateHandler", new SimpleOrderedMap<>(getUpdateHandlerInfo())); |
There was a problem hiding this comment.
I suspect you could place the MapWriter directly into the value of EntryWriter, without the SimpleOrderedMap copy. In the context of implementing writeMap (as is done here), this should be universally true, I think.
| new SimpleOrderedMap<>( | ||
| m -> { | ||
| new NamedList<>(items).writeMap(m); | ||
| })); |
There was a problem hiding this comment.
can't this be simply items I think (as it was) -- why the double conversion to NamedList and then to SimpleOrderedMap?
| for (PluginInfo info : infos) l.add(info); | ||
| result.put(tag, l); | ||
| ArrayList<MapWriter> writers = new ArrayList<>(); | ||
| infos.forEach(info -> writers.add(new SimpleOrderedMap<>(info))); |
There was a problem hiding this comment.
use of SimpleOrderedMap copy constructor / conversion seems needless
And same for other side of the if condition
| ew.put(tag, writers); | ||
| } else { | ||
| result.put(tag, infos.get(0)); | ||
| ew.put(tag, new SimpleOrderedMap<>(m -> infos.getFirst().writeMap(m))); |
| return result; | ||
| ew.put( | ||
| "requestDispatcher", | ||
| new SimpleOrderedMap<>( |
There was a problem hiding this comment.
The purpose of MapWriter is to write a map as efficiently as possible, ideally without an intermediary forced map (or NamedList / SimpleOrderedMap or other data structure conversions/copies). Prior to MapWriter, Solr used MapSerializable which forced the creation of maps.
I'll skip repeating this point but it applies to many changes in this commit.
|
|
||
| public Map<String, Object> getNamedPropertyValues(String name, SolrParams params) { | ||
| return new SchemaProps(name, params, this).toMap(new LinkedHashMap<>()); | ||
| return new SimpleOrderedMap<>(new SchemaProps(name, params, this)); |
There was a problem hiding this comment.
Use LinkedHashMap here as this method isn't apparently isolated for serialization
| // TODO: Should not create new HashMap? | ||
| return new HashMap<>(args); | ||
| public void writeMap(EntryWriter ew) throws IOException { | ||
| new NamedList<>(args).writeMap(ew); |
There was a problem hiding this comment.
surely we can do better (no NamedList creatoin). note the comment that was deleted -- same concept.
| .putIfNotNull("mergeScheduler", mergeSchedulerInfo) | ||
| .putIfNotNull("metrics", metricsInfo) | ||
| .putIfNotNull("mergePolicyFactory", mergePolicyFactoryInfo) | ||
| .putIfNotNull("mergedSegmentWarmer", mergedSegmentWarmerInfo); |
This is Part 2 of 4 to deprecate and remove the
MapSerializableinterface as part of SOLR-17600.This PR migrates Core Configuration classes (e.g.,
SolrConfig,PluginInfo,ConfigOverlay) to useMapWriter.Note: This PR is part of a stacked series and is built on top of Part 1 (#4463). It currently displays the commits from Part 1 as well, but they will automatically drop from this PR's diff once Part 1 is merged into
main.