isaric commented on code in PR #4463:
URL: https://github.com/apache/solr/pull/4463#discussion_r3367287764
##########
solr/core/src/java/org/apache/solr/handler/admin/api/OverseerOperationAPI.java:
##########
@@ -59,7 +59,7 @@ public OverseerOperationAPI(CoreAdminHandler
coreAdminHandler) {
@Command(name = OVERSEER_OP_CMD)
public void joinOverseerLeaderElection(PayloadObj<OverseerOperationPayload>
payload)
throws Exception {
- final Map<String, Object> v1Params = payload.get().toMap(new HashMap<>());
+ final Map<String, Object> v1Params = new SimpleOrderedMap<>(payload.get());
Review Comment:
Done — `Utils.convertToMap(payload.get(), new HashMap<>())`. The other
`*API` dispatch classes (`SplitShardAPI`, `MoveReplicaAPI`,
`RequestSyncShardAPI`, etc.) already follow this pattern.
##########
solr/core/src/java/org/apache/solr/handler/admin/BaseHandlerApiSupport.java:
##########
@@ -195,26 +196,32 @@ public Map<String, Object> toMap(Map<String, Object>
suppliedMap) {
: map.get(key);
if (o == null) o = pathValues.get(key);
if (o == null && useRequestParams) o = origParams.getParams(key);
+ if (o == null) {
+ ew.put(param, null);
+ continue;
+ }
Review Comment:
Not test-driven — the original `toMap` would NPE on `o.getClass()` two lines
down when all three lookups (`map.get(key)`, `pathValues.get(key)`,
`origParams.getParams(key)`) returned `null`. The `continue` is a latent-bug
fix. Downstream V1 handlers already treat absent and null-valued params the
same way, so semantics are preserved.
##########
solr/core/src/java/org/apache/solr/handler/admin/IndexSizeEstimator.java:
##########
@@ -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);
Review Comment:
Agreed — keeping SOM.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -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), new
SimpleOrderedMap<>(p)));
Review Comment:
Per your top-level comment about passing `MapWriter` directly, I dropped the
`new SimpleOrderedMap<>(p)` wrapper — `p` (a `ParamSet`, which is a
`MapWriter`) now flows straight through. Same end result, one less copy.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -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)
Review Comment:
Keeping SOM here. The resulting `pluginInfo` map is mutated downstream in
`getComponentInfo` (`computeIfAbsent`) and to inject `_packageinfo_` via `put`,
so it needs to be a real mutable `Map`.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
Review Comment:
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` / `setValue` on the map, so they need a mutable `Map`
and the conversion stays.
Note: 339 and 372 were also broken in the originally pushed code (tried to
wrap a `MapSerializable` in `SimpleOrderedMap`, which only has a `MapWriter`
constructor — 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 once `SolrConfig` / `PluginInfo` become
`MapWriter`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
