diff --git a/changelog/unreleased/SOLR-18603-seal-namedlist.yml b/changelog/unreleased/SOLR-18603-seal-namedlist.yml new file mode 100644 index 000000000000..7364b1708722 --- /dev/null +++ b/changelog/unreleased/SOLR-18603-seal-namedlist.yml @@ -0,0 +1,14 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: > + NamedList is now a sealed class whose only subclass is SimpleOrderedMap, which is final. Third-party + code can no longer extend either. CloudSolrClient.RouteResponse is removed: a routed update now + returns a plain SimpleOrderedMap whose responseHeader carries each route's achieved replication + factor under "rfByRoute", which getShardReplicationFactor() reads. +type: changed +authors: + - name: Ivan Šarić + nick: isaric +links: + - name: SOLR-18603 + url: https://issues.apache.org/jira/browse/SOLR-18603 diff --git a/solr/core/src/test/org/apache/solr/search/facet/DistributedFacetSimpleRefinementLongTailTest.java b/solr/core/src/test/org/apache/solr/search/facet/DistributedFacetSimpleRefinementLongTailTest.java index 34e4d6461204..9ffb64f6c3bb 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/DistributedFacetSimpleRefinementLongTailTest.java +++ b/solr/core/src/test/org/apache/solr/search/facet/DistributedFacetSimpleRefinementLongTailTest.java @@ -200,7 +200,7 @@ private void sanityCheckIndividualShards() throws Exception { { List bar_buckets = (List) - ((NamedList) shardFooBuckets[2].get(6).get("bar")).get("buckets"); + ((NamedList) shardFooBuckets[2].get(6).get("bar")).get("buckets"); assertEquals(6, bar_buckets.size()); for (int j = 0; j < 5; j++) { NamedList bucket = bar_buckets.get(j); diff --git a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc index ac2de0f81376..eb2e7b108abb 100644 --- a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc +++ b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc @@ -41,3 +41,14 @@ bin/solr start -Dsolr.node.roles=data:on,overseer:preferred A node started this way asks the Overseer to re-run its node prioritization, so a preferred node takes over without waiting for the current Overseer to restart. Note that node roles are fixed for the lifetime of a node: unlike `ADDROLE`, they cannot be changed on a running node. + +== SolrJ + +=== NamedList is a sealed class + +`NamedList` is now a sealed class whose only permitted subclass is `SimpleOrderedMap`, which is `final`. +Plugins or client code that extended either class will no longer compile; hold a `NamedList` rather than being one. + +`CloudSolrClient.RouteResponse` has been removed. +A routed update request now returns a plain `SimpleOrderedMap`, whose `responseHeader` carries the achieved replication factor of each route under `rfByRoute` (`CloudSolrClient.REPFACT_BY_ROUTE`), keyed by route URL. +`CloudSolrClient.getShardReplicationFactor()` reads that entry, so its signature and behavior are unchanged; `condenseResponse()` now returns a `SimpleOrderedMap`. diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index 07c8af0c5cd0..236db54011d5 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -29,7 +29,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -46,7 +45,6 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Supplier; import java.util.stream.Collectors; import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; @@ -414,13 +412,8 @@ private NamedList directUpdate(UpdateRequest request, String collection) long end = System.nanoTime(); - @SuppressWarnings({"rawtypes"}) - RouteResponse rr = - condenseResponse( - shardResponses, (int) TimeUnit.MILLISECONDS.convert(end - start, TimeUnit.NANOSECONDS)); - rr.setRouteResponses(shardResponses); - rr.setRoutes(routes); - return rr; + return condenseResponse( + shardResponses, (int) TimeUnit.MILLISECONDS.convert(end - start, TimeUnit.NANOSECONDS)); } protected RouteException getRouteException( @@ -487,11 +480,22 @@ private Map> buildUrlMap( return urlMap; } - protected > T condenseResponse( - NamedList response, int timeMillis, Supplier supplier) { - T condensed = supplier.get(); + /** + * Key in the merged {@code responseHeader} of a routed update request: the achieved replication + * factor of each route, keyed by route URL. + */ + public static final String REPFACT_BY_ROUTE = "rfByRoute"; + + /** + * Merges the per-route responses of a routed update request into a single response. The merged + * {@code responseHeader} carries the lowest achieved replication factor as {@link + * UpdateRequest#REPFACT} and each route's own value under {@link #REPFACT_BY_ROUTE}. + */ + public SimpleOrderedMap condenseResponse(NamedList response, int timeMillis) { + SimpleOrderedMap condensed = new SimpleOrderedMap<>(); int status = 0; Integer rf = null; + SimpleOrderedMap rfByRoute = null; // TolerantUpdateProcessor List> toleratedErrors = null; @@ -511,6 +515,8 @@ protected > T condenseResponse( Object rfObj = header.get(UpdateRequest.REPFACT); if (rfObj != null && rfObj instanceof Integer routeRf) { if (rf == null || routeRf < rf) rf = routeRf; + if (rfByRoute == null) rfByRoute = new SimpleOrderedMap<>(); + rfByRoute.put(response.getName(i), routeRf); } @SuppressWarnings("unchecked") @@ -549,6 +555,7 @@ protected > T condenseResponse( cheader.add("status", status); cheader.add("QTime", timeMillis); if (rf != null) cheader.add(UpdateRequest.REPFACT, rf); + if (rfByRoute != null) cheader.add(REPFACT_BY_ROUTE, rfByRoute); if (null != toleratedErrors) { cheader.add("maxErrors", ToleratedUpdateError.getUserFriendlyMaxErrors(maxToleratedErrors)); cheader.add("errors", toleratedErrors); @@ -584,11 +591,6 @@ protected > T condenseResponse( return condensed; } - @SuppressWarnings({"rawtypes"}) - public RouteResponse condenseResponse(NamedList response, int timeMillis) { - return condenseResponse(response, timeMillis, RouteResponse::new); - } - @Override public NamedList request(SolrRequest request, String collection) throws SolrServerException, IOException { @@ -1241,14 +1243,15 @@ public int getMinAchievedReplicationFactor(String collection, NamedList resp) } /** - * Walks the NamedList response after performing an update request looking for the replication - * factor that was achieved in each shard involved in the request. For single doc updates, there + * Reads the replication factor that was achieved in each shard involved in a routed update + * request, from {@link #REPFACT_BY_ROUTE} in the response header. For single doc updates, there * will be only one shard in the return value. */ public Map getShardReplicationFactor(String collection, NamedList resp) { Map results = new HashMap<>(); - if (resp instanceof RouteResponse) { - NamedList> routes = ((RouteResponse) resp).getRouteResponses(); + NamedList header = (NamedList) resp.get("responseHeader"); + Object rfByRoute = header == null ? null : header.get(REPFACT_BY_ROUTE); + if (rfByRoute instanceof NamedList routes) { DocCollection coll = getDocCollection(collection, null); Map leaders = new HashMap<>(); for (Slice slice : coll.getActiveSlices()) { @@ -1261,13 +1264,9 @@ public Map getShardReplicationFactor(String collection, NamedLi } } - Iterator>> routeIter = routes.iterator(); - while (routeIter.hasNext()) { - Map.Entry> next = routeIter.next(); + for (Map.Entry next : routes) { String host = next.getKey(); - NamedList hostResp = next.getValue(); - Integer rf = - (Integer) ((NamedList) hostResp.get("responseHeader")).get(UpdateRequest.REPFACT); + Integer rf = (Integer) next.getValue(); if (rf != null) { String shard = leaders.get(host); if (shard == null) { @@ -1681,28 +1680,6 @@ void evictStale() { } } - @SuppressWarnings({"rawtypes"}) - public static class RouteResponse extends NamedList { - private NamedList> routeResponses; - private Map routes; - - public void setRouteResponses(NamedList> routeResponses) { - this.routeResponses = routeResponses; - } - - public NamedList> getRouteResponses() { - return routeResponses; - } - - public void setRoutes(Map routes) { - this.routes = routes; - } - - public Map getRoutes() { - return routes; - } - } - public static class RouteException extends SolrException { private NamedList throwables; diff --git a/solr/solrj/src/java/org/apache/solr/common/util/NamedList.java b/solr/solrj/src/java/org/apache/solr/common/util/NamedList.java index 13afd38e428a..697970420a24 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/NamedList.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/NamedList.java @@ -53,10 +53,13 @@ * allows order to be easily preserved in various clients (i.e. not a straight map). If access by * key is more important for serialization, see {@link SimpleOrderedMap}, or simply use a regular * {@link Map} + * + *

This class is sealed: {@link SimpleOrderedMap} is its only direct subclass. */ @SuppressWarnings({"unchecked", "rawtypes"}) -public class NamedList - implements Cloneable, Serializable, Iterable>, MapWriter { +public sealed class NamedList + implements Cloneable, Serializable, Iterable>, MapWriter + permits SimpleOrderedMap { private static final long serialVersionUID = 1957981902839867821L; protected final List nvPairs; diff --git a/solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java b/solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java index d97990c35e9b..e00870603772 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java @@ -43,7 +43,7 @@ * hold data to be serialized. It aims to minimize overhead and to be efficient at adding new * elements. */ -public class SimpleOrderedMap extends NamedList implements Map { +public final class SimpleOrderedMap extends NamedList implements Map { private static final SimpleOrderedMap EMPTY = new SimpleOrderedMap<>(List.of()); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java index ee80cbb85b9c..7f4a7971779a 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java @@ -18,7 +18,6 @@ import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST; import static org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider.SYS_PROP_CACHE_TIMEOUT_SECONDS; -import static org.apache.solr.client.solrj.impl.CloudSolrClient.RouteResponse; import java.io.IOException; import java.lang.invoke.MethodHandles; @@ -361,6 +360,54 @@ private CloudSolrClient createHttpCSPBasedCloudSolrClient() { return new CloudSolrClient.Builder(solrUrls).build(); } + @Test + public void testShardReplicationFactors() throws Exception { + final String collection = "replication_factor_collection"; + CloudSolrClient client = cluster.getSolrClient(); + CollectionAdminRequest.createCollection(collection, "conf", 2, 1).process(client); + cluster.waitForActiveCollection(collection, 2, 2); + + DocCollection col = client.getClusterState().getCollection(collection); + String shard1Url = col.getSlice("shard1").getLeader().getCoreUrl(); + String shard2Url = col.getSlice("shard2").getLeader().getCoreUrl(); + NamedList> routeResponses = new NamedList<>(); + routeResponses.add( + shard1Url, + new NamedList<>( + Map.of( + "responseHeader", new NamedList<>(Map.of("status", 0, UpdateRequest.REPFACT, 2))))); + routeResponses.add( + shard2Url, + new NamedList<>( + Map.of( + "responseHeader", new NamedList<>(Map.of("status", 0, UpdateRequest.REPFACT, 3))))); + + NamedList response = client.condenseResponse(routeResponses, 7); + NamedList header = (NamedList) response.get("responseHeader"); + assertEquals(Map.of(shard1Url, 2, shard2Url, 3), header.get(CloudSolrClient.REPFACT_BY_ROUTE)); + assertEquals( + Map.of("shard1", 2, "shard2", 3), client.getShardReplicationFactor(collection, response)); + assertEquals(2, client.getMinAchievedReplicationFactor(collection, response)); + } + + @Test + public void testShardReplicationFactorsWithoutRouteFactors() { + CloudSolrClient client = cluster.getSolrClient(); + NamedList> routeResponses = new NamedList<>(); + routeResponses.add( + "http://unused/solr/core", + new NamedList<>(Map.of("responseHeader", new NamedList<>(Map.of("status", 0))))); + + NamedList response = client.condenseResponse(routeResponses, 7); + NamedList header = (NamedList) response.get("responseHeader"); + assertNull(header.get(CloudSolrClient.REPFACT_BY_ROUTE)); + assertNull(header.get(UpdateRequest.REPFACT)); + assertEquals(Map.of(), client.getShardReplicationFactor("missing_collection", response)); + assertEquals(-1, client.getMinAchievedReplicationFactor("missing_collection", response)); + assertEquals( + Map.of(), client.getShardReplicationFactor("missing_collection", new NamedList<>())); + } + @Test public void testRouting() throws Exception { CollectionAdminRequest.createCollection("routing_collection", "conf", 2, 1) @@ -378,29 +425,7 @@ public void testRouting() throws Exception { if (getRandomClient().isDirectUpdatesToLeadersOnly()) { checkSingleServer(response); } - RouteResponse rr = (RouteResponse) response; - Map routes = rr.getRoutes(); - Iterator> it = - routes.entrySet().iterator(); - while (it.hasNext()) { - Map.Entry entry = it.next(); - String coreUrl = entry.getKey(); - final String baseUrl = URLUtil.extractBaseUrl(coreUrl); - final String coreName = URLUtil.extractCoreFromCoreUrl(coreUrl); - UpdateRequest updateRequest = (UpdateRequest) entry.getValue().getRequest(); - SolrInputDocument doc = updateRequest.getDocuments().get(0); - String id = doc.getField("id").getValue().toString(); - ModifiableSolrParams params = new ModifiableSolrParams(); - params.add("q", "id:" + id); - params.add("distrib", "false"); - QueryRequest queryRequest = new QueryRequest(params); - - SolrClient solrClient = - new CollectionScopedSolrClient(cluster.getJetty(baseUrl).getSolrClient(), coreName); - QueryResponse queryResponse = queryRequest.process(solrClient); - SolrDocumentList docList = queryResponse.getResults(); - assertEquals(1, docList.getNumFound()); - } + assertDocsOnShardLeaders("routing_collection", "0", "2"); // Test the deleteById routing for UpdateRequest @@ -429,27 +454,7 @@ public void testRouting() throws Exception { if (threadedClient.isDirectUpdatesToLeadersOnly()) { checkSingleServer(response); } - rr = (RouteResponse) response; - routes = rr.getRoutes(); - it = routes.entrySet().iterator(); - while (it.hasNext()) { - Map.Entry entry = it.next(); - String coreUrl = entry.getKey(); - final String baseUrl = URLUtil.extractBaseUrl(coreUrl); - final String coreName = URLUtil.extractCoreFromCoreUrl(coreUrl); - UpdateRequest updateRequest = (UpdateRequest) entry.getValue().getRequest(); - SolrInputDocument doc = updateRequest.getDocuments().get(0); - String id = doc.getField("id").getValue().toString(); - ModifiableSolrParams params = new ModifiableSolrParams(); - params.add("q", "id:" + id); - params.add("distrib", "false"); - QueryRequest queryRequest = new QueryRequest(params); - SolrClient solrClient = - new CollectionScopedSolrClient(cluster.getJetty(baseUrl).getSolrClient(), coreName); - QueryResponse queryResponse = queryRequest.process(solrClient); - SolrDocumentList docList = queryResponse.getResults(); - assertEquals(1, docList.getNumFound()); - } + assertDocsOnShardLeaders("routing_collection", "0", "2"); } // Test that queries with _route_ params are routed by the client @@ -1096,18 +1101,40 @@ public void testRetryUpdatesWhenClusterStateIsStale() throws Exception { } } + /** Asserts that each doc is on the leader of the shard it routes to, querying that core alone. */ + private static void assertDocsOnShardLeaders(String collection, String... ids) throws Exception { + DocCollection col = cluster.getSolrClient().getClusterState().getCollection(collection); + for (String id : ids) { + String coreUrl = + col.getRouter().getTargetSlice(id, null, null, null, col).getLeader().getCoreUrl(); + QueryResponse queryResponse = + new QueryRequest(params("q", "id:" + id, "distrib", "false")) + .process( + cluster.getJetty(URLUtil.extractBaseUrl(coreUrl)).getSolrClient(), + URLUtil.extractCoreFromCoreUrl(coreUrl)); + assertEquals(1, queryResponse.getResults().getNumFound()); + } + } + + /** Asserts that a routed update was sent to shard leaders only, via the per-route rf header. */ private static void checkSingleServer(NamedList response) { - final RouteResponse rr = (RouteResponse) response; - final Map routes = rr.getRoutes(); - final Iterator> it = - routes.entrySet().iterator(); - while (it.hasNext()) { - Map.Entry entry = it.next(); - assertEquals( - "wrong number of servers: " + entry.getValue().getEndpoints(), - 1, - entry.getValue().getEndpoints().size()); + NamedList header = (NamedList) response.get("responseHeader"); + NamedList rfByRoute = (NamedList) header.get(CloudSolrClient.REPFACT_BY_ROUTE); + assertNotNull("no per-route rf in " + header, rfByRoute); + DocCollection col = + cluster.getSolrClient().getClusterState().getCollection("routing_collection"); + Set leaderUrls = new HashSet<>(); + for (Slice slice : col.getSlices()) { + leaderUrls.add(slice.getLeader().getCoreUrl()); } + assertEquals("missing routes in " + rfByRoute, leaderUrls.size(), rfByRoute.size()); + for (Map.Entry route : rfByRoute) { + assertTrue("not a leader: " + route.getKey(), leaderUrls.contains(route.getKey())); + assertEquals(Integer.valueOf(1), route.getValue()); + } + assertEquals( + Map.of("shard1", 1, "shard2", 1), + cluster.getSolrClient().getShardReplicationFactor("routing_collection", response)); } /** diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/response/NestableJsonFacetTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/response/NestableJsonFacetTest.java index e9230c358e0a..2e54375bbc60 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/response/NestableJsonFacetTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/response/NestableJsonFacetTest.java @@ -21,6 +21,7 @@ import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.client.solrj.response.json.NestableJsonFacet; import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.SimpleOrderedMap; import org.junit.Test; public class NestableJsonFacetTest extends SolrTestCaseJ4 { @@ -29,46 +30,14 @@ public class NestableJsonFacetTest extends SolrTestCaseJ4 { public void testParsing() { NamedList list = new NamedList<>(); list.add("count", 12); - NamedList buckets = - new NamedList<>() { - { - add("val", "Nike"); - } - }; - NamedList vals = - new NamedList<>() { - { - add("numBuckets", 10); - add( - "allBuckets", - new NamedList<>() { - { - add("count", 12); - } - }); - add( - "before", - new NamedList<>() { - { - add("count", 1); - } - }); - add( - "after", - new NamedList<>() { - { - add("count", 2); - } - }); - add( - "between", - new NamedList<>() { - { - add("count", 9); - } - }); - } - }; + NamedList buckets = new NamedList<>(); + buckets.add("val", "Nike"); + NamedList vals = new NamedList<>(); + vals.add("numBuckets", 10); + vals.add("allBuckets", SimpleOrderedMap.of("count", 12)); + vals.add("before", SimpleOrderedMap.of("count", 1)); + vals.add("after", SimpleOrderedMap.of("count", 2)); + vals.add("between", SimpleOrderedMap.of("count", 9)); vals.add("buckets", List.of(buckets)); list.add("test", vals); NestableJsonFacet facet = new NestableJsonFacet(list); @@ -78,46 +47,14 @@ public void testParsing() { list.clear(); list.add("count", 12L); - buckets = - new NamedList<>() { - { - add("val", "Nike"); - } - }; - vals = - new NamedList<>() { - { - add("numBuckets", 10L); - add( - "allBuckets", - new NamedList<>() { - { - add("count", 12L); - } - }); - add( - "before", - new NamedList<>() { - { - add("count", 1L); - } - }); - add( - "after", - new NamedList<>() { - { - add("count", 2L); - } - }); - add( - "between", - new NamedList<>() { - { - add("count", 9L); - } - }); - } - }; + buckets = new NamedList<>(); + buckets.add("val", "Nike"); + vals = new NamedList<>(); + vals.add("numBuckets", 10L); + vals.add("allBuckets", SimpleOrderedMap.of("count", 12L)); + vals.add("before", SimpleOrderedMap.of("count", 1L)); + vals.add("after", SimpleOrderedMap.of("count", 2L)); + vals.add("between", SimpleOrderedMap.of("count", 9L)); vals.add("buckets", List.of(buckets)); list.add("test", vals); facet = new NestableJsonFacet(list);