dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3837490732
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -127,11 +128,11 @@ public static NodeConfig fromConfig(
// It should go inside the fillSolrSection method but
// since it is arranged as a separate section it is placed here
Map<String, String> coreAdminHandlerActions =
- readNodeListAsNamedList(root.get("coreAdminHandlerActions"),
"<coreAdminHandlerActions>")
- .asShallowMap()
- .entrySet()
- .stream()
- .collect(Collectors.toMap(Entry::getKey, item ->
item.getValue().toString()));
+ new SimpleOrderedMap<>(
Review Comment:
no; do not use SimpleOrderedMap as a general purpose Map. It's not
documented well but we should only be creating new ones when we are writing
response data structures for efficiency reasons.
##########
solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java:
##########
Review Comment:
I filed https://issues.apache.org/jira/browse/SOLR-18398 for Solr 11 and
assigned to me.
Related to that -- I see that a number of SolrQueryResponse getter's return
a NamedList that are in fact known to be a SimpleOrderedMap. I think it's fair
to return the correct type. Granted that'd be a big enough change that it'd
deserve it's own PR first. So what that in place, the logic here is basically
one line -- call `put`. It will apply to more places in this PR likewise.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -56,7 +57,8 @@ public class JavaBinUpdateRequestCodec {
public void marshal(UpdateRequest updateRequest, OutputStream os) throws
IOException {
NamedList<Object> nl = new NamedList<>();
- NamedList<Object> params = updateRequest.getParams().toNamedList();
+ // SimpleOrderedMap serializes with the same JavaBin ORDERED_MAP tag as
before.
Review Comment:
"as before" is PR language to communicate a change but is confusing /
inappropriate to actually commit
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -421,16 +422,14 @@ private Pair<List<String>, List<String>>
deployCollectionPackage(
// Get package params
try {
- boolean packageParamsExist =
- solrClient
- .request(
- new GenericV2SolrRequest(
- SolrRequest.METHOD.GET,
- PackageUtils.getCollectionParamsPath(collection) +
"/packages")
- .setRequiresCollection(
- false) /* Making a collection-request, but already
baked into path */)
- .asShallowMap()
- .containsKey("params");
+ NamedList<Object> collectionParams =
+ solrClient.request(
+ new GenericV2SolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) +
"/packages")
+ .setRequiresCollection(
+ false) /* Making a collection-request, but already
baked into path */);
+ boolean packageParamsExist = new
SimpleOrderedMap<>(collectionParams).containsKey("params");
Review Comment:
this is very sad BTW; just call `collectionParams.indexOf("params") >= 0`
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -70,7 +70,8 @@ public NamedListSerializer(Class<NamedList> nlClazz) {
@Override
public void serialize(NamedList value, JsonGenerator gen,
SerializerProvider provider)
throws IOException {
- gen.writeObject(value.asShallowMap());
+ // Not SimpleOrderedMap: it IS a NamedList, so this serializer would
recurse on it.
+ gen.writeObject(value.asMap(0));
Review Comment:
can you elaborate with more words 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: [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]
