You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "isaric (via GitHub)" <gi...@apache.org> on 2026/01/06 09:19:40 UTC
[PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
isaric opened a new pull request, #4011:
URL: https://github.com/apache/solr/pull/4011
https://issues.apache.org/jira/browse/SOLR-17600
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
### Description
This pull request replaces the deprecated `MapSerializable` interface with `MapWriter` across the Solr codebase. `MapSerializable` relied on the `toMap(Map)` pattern, which often led to unnecessary object allocations and inconsistent serialization behavior. By migrating to `MapWriter` and its `writeMap(EntryWriter)` pattern, we improve memory efficiency and ensure more consistent serialization across different formats like JavaBin and JSON.
### Solution
The migration involved several key steps:
1. **Interface Removal**: Deleted the `MapSerializable` interface and removed its usage from the `MapWriter` interface. `MapWriter` now serves as the primary interface for object-to-map serialization.
2. **Refactoring Components**: Updated numerous classes across the codebase to implement `MapWriter` instead of `MapSerializable`. Key areas include:
* **Core Configuration**: `SolrConfig`, `PluginInfo`, `SolrIndexConfig`, `CacheConfig`, `ConfigOverlay`, `RequestParams`, `IndexSchema`.
* **Cloud & Admin APIs**: Many V2 API response classes such as `CreateCore`, `InstallShardData`, `MigrateDocsAPI`, `ModifyCollectionAPI`, `MoveReplicaAPI`, `SplitShardAPI`, and more.
* **Internal State**: `ZkNodeProps`, `IndexFingerprint`.
3. **Method Migration**: Converted `toMap(Map<String, Object> map)` implementations to `writeMap(EntryWriter ew)`. This allows for direct writing to the underlying data structure (like `JavaBinCodec`'s output stream) without necessarily creating intermediate `Map` objects.
4. **Utility Updates**: Updated `Utils.convertToMap`, `Utils.reflectToMap`, and other utility methods to support the new pattern. `Utils.makeDeepCopy` now uses `SimpleOrderedMap` with `MapWriter` for better efficiency.
5. **Clean up**: Removed redundant `toMap` calls and simplified serialization logic in several handlers and components. Updated `JavaBinCodec`, `TextWriter`, and `JSONWriter` to optimize for `MapWriter`.
### Tests
Existing tests were updated to accommodate the change in serialization:
* Updated `TestConfig`, `SolrIndexConfigTest`, `CacheConfigTest`, and `NodeConfigClusterPluginsSourceTest` to verify that the configuration objects are still correctly serialized.
* Updated `TestSchemaDesignerAPI` and `SolrExampleCborTest` to ensure API and serialization consistency.
* Verified that `JavaBinCodec` and `JSONWriter` correctly handle the new `MapWriter` implementations.
* Ran core Solr tests to ensure no regressions in response formatting or internal state representation.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
- [ ] I have added a [changelog entry](https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc) for my change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4011:
URL: https://github.com/apache/solr/pull/4011#issuecomment-4094555827
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2678317636
##########
solr/core/src/java/org/apache/solr/handler/admin/api/InstallShardData.java:
##########
@@ -129,6 +130,6 @@ public static ZkNodeProps createRemoteMessage(
}
messageTyped.validate();
- return new ZkNodeProps(messageTyped.toMap(new HashMap<>()));
+ return new ZkNodeProps((Map<String, Object>) new SimpleOrderedMap<>(messageTyped));
Review Comment:
generally, we should avoid creating a SimpleOrderedMap. Use Utils.convertToMap instead.
A SimpleOrderedMap is okay to add to a MapWritable entry though, or any similar place for a response-writing use-case. A SimpleOrderedMap does not hash its data, so has O(N) access. It's designed for writing out data that will unlikely have its keys looked up. It's not a general replacement for a Map.
##########
changelog/unreleased/SOLR-17600.yml:
##########
@@ -0,0 +1,7 @@
+title: SOLR 17600 - Replace MapSerializable with MapWriter
+type: changed
Review Comment:
refactorings are "other".
I'm not happy with the logchange plugin constraining the enum choices here
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -981,45 +991,47 @@ public Map<String, Object> toMap(Map<String, Object> result) {
overlay.getNamedPlugins(plugin.tag).entrySet()) {
items.put(e.getKey(), e.getValue());
}
- result.put(tag, items);
+ ew.put(
+ tag,
+ new SimpleOrderedMap<>(
+ m -> {
+ new NamedList<>(items).writeMap(m);
+ }));
Review Comment:
not sure what I'm looking at here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2667219432
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CreateCore.java:
##########
@@ -158,7 +159,7 @@ private Map<String, String> buildCoreParams(CreateCoreParams createParams) {
}
public static CreateCoreParams createRequestBodyFromV1Params(SolrParams solrParams) {
- final var v1ParamMap = solrParams.toMap(new HashMap<>());
+ final var v1ParamMap = new SimpleOrderedMap<>(solrParams);
Review Comment:
There is a subtle semantic difference here. Previously, we had a bonafide Map. If there were duplicate keys, the last value wins (looking at Utils.convertToMap, called by toMap). Probably wasn't intended but there is integrity of the Map contract at least. The constructor you use assumes the input has no duplicate keys. They will be added, and SimpleOrderedMap will not quite act exactly like a Map. I'm not sure what the final effect is without trying/testing. We could change SimpleOrderedMap's constructor here to have a hack for the NamedList (that isn't SimpleOrderedMap) case. Or don't use SimpleOrderedMap here, which I can see was chosen for your convenience, not because it's needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on PR #4011:
URL: https://github.com/apache/solr/pull/4011#issuecomment-3729846737
@dsmiley I followed the advice in the comments however I had test failures in solr:core after pushing changes. Reverted changes to PluginInfo and tests are passing on my Codespaces instance. I don't think the solr:core tests ran for the last commit. I guess it is restricted due to heavy resource use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2701818301
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -951,20 +946,35 @@ public int getFormUploadLimitKB() {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> result) {
- if (znodeVersion > -1) result.put(ZNODEVER, znodeVersion);
- if (luceneMatchVersion != null)
- result.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM, luceneMatchVersion.toString());
- result.put("updateHandler", getUpdateHandlerInfo());
- Map<String, Object> m = new LinkedHashMap<>();
- result.put("query", m);
- m.put("useFilterForSortedQuery", useFilterForSortedQuery);
- m.put("queryResultWindowSize", queryResultWindowSize);
- m.put("queryResultMaxDocsCached", queryResultMaxDocsCached);
- m.put("enableLazyFieldLoading", enableLazyFieldLoading);
- m.put("maxBooleanClauses", booleanQueryMaxClauseCount);
- m.put(MIN_PREFIX_QUERY_TERM_LENGTH, prefixQueryMinPrefixLength);
+ public void writeMap(EntryWriter ew) throws IOException {
+ if (znodeVersion > -1) {
+ ew.put(ZNODEVER, znodeVersion);
+ }
+ if (luceneMatchVersion != null) {
+ ew.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM, luceneMatchVersion.toString());
+ }
+ ew.put("updateHandler", new SimpleOrderedMap<>(getUpdateHandlerInfo()));
+ ew.put(
+ "query",
+ new SimpleOrderedMap<>(
+ (MapWriter)
+ m -> {
Review Comment:
I suspect you could pass the MapWriter as a value to `put("query", ...)` without the SimpleOrderedMap wrapper. Try that please -- I suspect this works (or we could enhance MapWriter to make it work if it doesn't). I just did some looking and I think we should go this route.
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -981,45 +991,47 @@ public Map<String, Object> toMap(Map<String, Object> result) {
overlay.getNamedPlugins(plugin.tag).entrySet()) {
items.put(e.getKey(), e.getValue());
}
- result.put(tag, items);
+ ew.put(
+ tag,
+ new SimpleOrderedMap<>(
+ m -> {
+ new NamedList<>(items).writeMap(m);
+ }));
Review Comment:
Can't this be `eq.put(tag, items)` ? If that's basically what it was before then not sure why we add double intermediation of SOM & NL.
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -193,27 +195,19 @@ public PluginInfo getChild(String type) {
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.putAll(attributes);
- Map m = map;
- if (initArgs != null) m.putAll(initArgs.asMap(3));
- if (children != null) {
- for (PluginInfo child : children) {
- Object old = m.get(child.name);
- if (old == null) {
- m.put(child.name, child.toMap(new LinkedHashMap<>()));
- } else if (old instanceof List list) {
- list.add(child.toMap(new LinkedHashMap<>()));
- } else {
- ArrayList l = new ArrayList();
- l.add(old);
- l.add(child.toMap(new LinkedHashMap<>()));
- m.put(child.name, l);
- }
- }
+ @SuppressWarnings("unchecked")
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(attributes).writeMap(ew);
+ if (initArgs != null) {
+ new SimpleOrderedMap<>(initArgs).writeMap(ew);
Review Comment:
Implementations of `writeMap` should strongly avoid this pattern -- creating a SimpleOrderedMap (or any data structure) merely to call writeMap / write it. The very point of `MapWriter` being introduced, over `MapSerializable` was to avoid this.
I'll see if I can help here
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -951,20 +946,35 @@ public int getFormUploadLimitKB() {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> result) {
- if (znodeVersion > -1) result.put(ZNODEVER, znodeVersion);
- if (luceneMatchVersion != null)
- result.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM, luceneMatchVersion.toString());
- result.put("updateHandler", getUpdateHandlerInfo());
- Map<String, Object> m = new LinkedHashMap<>();
- result.put("query", m);
- m.put("useFilterForSortedQuery", useFilterForSortedQuery);
- m.put("queryResultWindowSize", queryResultWindowSize);
- m.put("queryResultMaxDocsCached", queryResultMaxDocsCached);
- m.put("enableLazyFieldLoading", enableLazyFieldLoading);
- m.put("maxBooleanClauses", booleanQueryMaxClauseCount);
- m.put(MIN_PREFIX_QUERY_TERM_LENGTH, prefixQueryMinPrefixLength);
+ public void writeMap(EntryWriter ew) throws IOException {
+ if (znodeVersion > -1) {
+ ew.put(ZNODEVER, znodeVersion);
+ }
+ if (luceneMatchVersion != null) {
+ ew.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM, luceneMatchVersion.toString());
+ }
+ ew.put("updateHandler", new SimpleOrderedMap<>(getUpdateHandlerInfo()));
Review Comment:
previously there was no SimpleOrderedMap wrapper added around `getUpdateHandlerInfo`... so I'm wondering why add it now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on PR #4011:
URL: https://github.com/apache/solr/pull/4011#issuecomment-3764948526
> I keep seeing the need for me to repeat my same feedback, more or less.
>
> Do you think it'd be helpful to somehow reduce the scope and do a PR of that more narrow scope?
Hey @dsmiley , please dont consider every push as a request for review. Is it okay if I tag you when I'm ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2664731752
##########
solr/core/src/java/org/apache/solr/core/RequestParams.java:
##########
@@ -130,7 +132,9 @@ public RequestParams setParams(String name, ParamSet paramSet) {
Map p = (Map) deepCopy.get(NAME);
if (p == null) deepCopy.put(NAME, p = new LinkedHashMap<>());
if (paramSet == null) p.remove(name);
Review Comment:
I wonder if this if should have { } to match the else?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4011:
URL: https://github.com/apache/solr/pull/4011#issuecomment-4483445596
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2667418763
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CreateCore.java:
##########
@@ -158,7 +159,7 @@ private Map<String, String> buildCoreParams(CreateCoreParams createParams) {
}
public static CreateCoreParams createRequestBodyFromV1Params(SolrParams solrParams) {
- final var v1ParamMap = solrParams.toMap(new HashMap<>());
+ final var v1ParamMap = new SimpleOrderedMap<>(solrParams);
Review Comment:
Replaced with `Utils.convertToMap`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2664824463
##########
solr/core/src/java/org/apache/solr/core/RequestParams.java:
##########
@@ -130,7 +132,9 @@ public RequestParams setParams(String name, ParamSet paramSet) {
Map p = (Map) deepCopy.get(NAME);
if (p == null) deepCopy.put(NAME, p = new LinkedHashMap<>());
if (paramSet == null) p.remove(name);
Review Comment:
I agree that in Java conditionals should always use curly braces for clarity. This is the existing code that was not changed in my PR.
I am hesitant to change it as I am not sure if there is a convention regarding this practice in the project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4011: SOLR-17600 Replaces MapSerializable with MapWriter
URL: https://github.com/apache/solr/pull/4011
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2674922297
##########
solr/core/src/java/org/apache/solr/core/RequestParams.java:
##########
@@ -130,7 +132,9 @@ public RequestParams setParams(String name, ParamSet paramSet) {
Map p = (Map) deepCopy.get(NAME);
if (p == null) deepCopy.put(NAME, p = new LinkedHashMap<>());
if (paramSet == null) p.remove(name);
- else p.put(name, paramSet.toMap(new LinkedHashMap<>()));
+ else {
+ p.put(name, new SimpleOrderedMap<>(paramSet));
Review Comment:
This one's fine
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -193,27 +195,19 @@ public PluginInfo getChild(String type) {
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.putAll(attributes);
- Map m = map;
- if (initArgs != null) m.putAll(initArgs.asMap(3));
- if (children != null) {
- for (PluginInfo child : children) {
- Object old = m.get(child.name);
- if (old == null) {
- m.put(child.name, child.toMap(new LinkedHashMap<>()));
- } else if (old instanceof List list) {
- list.add(child.toMap(new LinkedHashMap<>()));
- } else {
- ArrayList l = new ArrayList();
- l.add(old);
- l.add(child.toMap(new LinkedHashMap<>()));
- m.put(child.name, l);
- }
- }
+ @SuppressWarnings("unchecked")
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(attributes).writeMap(ew);
+ if (initArgs != null) {
+ new SimpleOrderedMap<>(initArgs).writeMap(ew);
+ }
Review Comment:
Let's not create a new Map just to ultimately write it!
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/InstallShardDataCmd.java:
##########
@@ -80,8 +80,7 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
final ModifiableSolrParams coreApiParams = new ModifiableSolrParams();
coreApiParams.set(
CoreAdminParams.ACTION, CoreAdminParams.CoreAdminAction.INSTALLCOREDATA.toString());
- typedMessage.toMap(new HashMap<>()).forEach((k, v) -> coreApiParams.set(k, v.toString()));
-
+ new SimpleOrderedMap<>(typedMessage).forEach((k, v) -> coreApiParams.set(k, v.toString()));
Review Comment:
`typedMessage._forEachEntry` here instead
(avoiding senseless intermediate data structure creation).
##########
solr/core/src/java/org/apache/solr/api/NodeConfigClusterPluginsSource.java:
##########
@@ -79,7 +80,7 @@ private static Map<String, Object> readPlugins(final NodeConfig cfg) {
pluginMap.put("class", p.className);
if (p.initArgs.size() > 0) {
- Map<String, Object> config = p.initArgs.toMap(new HashMap<>());
+ Map<String, Object> config = new SimpleOrderedMap<>(p.initArgs);
Review Comment:
Again, avoid NamedList/SimpleOrderedMap
How about `p.initArgs.asMap(0)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2676508252
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/InstallShardDataCmd.java:
##########
@@ -80,8 +80,7 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
final ModifiableSolrParams coreApiParams = new ModifiableSolrParams();
coreApiParams.set(
CoreAdminParams.ACTION, CoreAdminParams.CoreAdminAction.INSTALLCOREDATA.toString());
- typedMessage.toMap(new HashMap<>()).forEach((k, v) -> coreApiParams.set(k, v.toString()));
-
+ new SimpleOrderedMap<>(typedMessage).forEach((k, v) -> coreApiParams.set(k, v.toString()));
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2676965442
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -193,27 +195,19 @@ public PluginInfo getChild(String type) {
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.putAll(attributes);
- Map m = map;
- if (initArgs != null) m.putAll(initArgs.asMap(3));
- if (children != null) {
- for (PluginInfo child : children) {
- Object old = m.get(child.name);
- if (old == null) {
- m.put(child.name, child.toMap(new LinkedHashMap<>()));
- } else if (old instanceof List list) {
- list.add(child.toMap(new LinkedHashMap<>()));
- } else {
- ArrayList l = new ArrayList();
- l.add(old);
- l.add(child.toMap(new LinkedHashMap<>()));
- m.put(child.name, l);
- }
- }
+ @SuppressWarnings("unchecked")
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(attributes).writeMap(ew);
+ if (initArgs != null) {
+ new SimpleOrderedMap<>(initArgs).writeMap(ew);
+ }
Review Comment:
Changed caused test failures. Unsure why. Error was cannot cast LinkedHashMap to String however stack trace was unclear as to location of exception.
Reverted change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2676537209
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/InstallShardDataCmd.java:
##########
@@ -80,8 +80,7 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
final ModifiableSolrParams coreApiParams = new ModifiableSolrParams();
coreApiParams.set(
CoreAdminParams.ACTION, CoreAdminParams.CoreAdminAction.INSTALLCOREDATA.toString());
- typedMessage.toMap(new HashMap<>()).forEach((k, v) -> coreApiParams.set(k, v.toString()));
-
+ new SimpleOrderedMap<>(typedMessage).forEach((k, v) -> coreApiParams.set(k, v.toString()));
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2676507478
##########
solr/core/src/java/org/apache/solr/api/NodeConfigClusterPluginsSource.java:
##########
@@ -79,7 +80,7 @@ private static Map<String, Object> readPlugins(final NodeConfig cfg) {
pluginMap.put("class", p.className);
if (p.initArgs.size() > 0) {
- Map<String, Object> config = p.initArgs.toMap(new HashMap<>());
+ Map<String, Object> config = new SimpleOrderedMap<>(p.initArgs);
Review Comment:
Changed as requested
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/InstallShardDataCmd.java:
##########
@@ -80,8 +80,7 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
final ModifiableSolrParams coreApiParams = new ModifiableSolrParams();
coreApiParams.set(
CoreAdminParams.ACTION, CoreAdminParams.CoreAdminAction.INSTALLCOREDATA.toString());
- typedMessage.toMap(new HashMap<>()).forEach((k, v) -> coreApiParams.set(k, v.toString()));
-
+ new SimpleOrderedMap<>(typedMessage).forEach((k, v) -> coreApiParams.set(k, v.toString()));
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "isaric (via GitHub)" <gi...@apache.org>.
isaric commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2676509212
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -193,27 +195,19 @@ public PluginInfo getChild(String type) {
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.putAll(attributes);
- Map m = map;
- if (initArgs != null) m.putAll(initArgs.asMap(3));
- if (children != null) {
- for (PluginInfo child : children) {
- Object old = m.get(child.name);
- if (old == null) {
- m.put(child.name, child.toMap(new LinkedHashMap<>()));
- } else if (old instanceof List list) {
- list.add(child.toMap(new LinkedHashMap<>()));
- } else {
- ArrayList l = new ArrayList();
- l.add(old);
- l.add(child.toMap(new LinkedHashMap<>()));
- m.put(child.name, l);
- }
- }
+ @SuppressWarnings("unchecked")
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(attributes).writeMap(ew);
+ if (initArgs != null) {
+ new SimpleOrderedMap<>(initArgs).writeMap(ew);
+ }
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17600 Replaces MapSerializable with MapWriter [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4011:
URL: https://github.com/apache/solr/pull/4011#discussion_r2664740615
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -684,23 +686,17 @@ public HttpCachingConfig getHttpCachingConfig() {
return httpCachingConfig;
}
- public static class HttpCachingConfig implements MapSerializable {
+ public static class HttpCachingConfig implements MapWriter {
/** For extracting Expires "ttl" from <cacheControl> config */
private static final Pattern MAX_AGE = Pattern.compile("\\bmax-age=(\\d+)");
@Override
- public Map<String, Object> toMap(Map<String, Object> map) {
- // Could have nulls
- return Utils.makeMap(
- "never304",
- never304,
- "etagSeed",
- etagSeed,
- "lastModFrom",
- lastModFrom.name().toLowerCase(Locale.ROOT),
- "cacheControl",
- cacheControlHeader);
+ public void writeMap(EntryWriter ew) throws IOException {
+ ew.put("never304", never304)
Review Comment:
this is nicer pattern, less a "unique magic pattern".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org