You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by gu...@apache.org on 2022/05/18 04:14:32 UTC
[solr] branch main updated: SOLR-16194 Don't overwrite list of collections in a Routed Alias when updating parameters. (#864)
This is an automated email from the ASF dual-hosted git repository.
gus pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new adeab2d88f5 SOLR-16194 Don't overwrite list of collections in a Routed Alias when updating parameters. (#864)
adeab2d88f5 is described below
commit adeab2d88f5bbb5bf56d5d4fc5fda349b9c8ee10
Author: Gus Heck <46...@users.noreply.github.com>
AuthorDate: Wed May 18 00:14:26 2022 -0400
SOLR-16194 Don't overwrite list of collections in a Routed Alias when updating parameters. (#864)
---
.../solr/cloud/api/collections/CreateAliasCmd.java | 42 ++++---
.../java/org/apache/solr/core/CoreContainer.java | 20 ++++
.../solr/handler/admin/CollectionsHandler.java | 31 +++---
.../java/org/apache/solr/servlet/HttpSolrCall.java | 8 +-
.../apache/solr/cloud/CreateRoutedAliasTest.java | 121 +++++++++++++++++----
.../deployment-guide/pages/alias-management.adoc | 34 +++---
6 files changed, 182 insertions(+), 74 deletions(-)
diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java
index e4cd3ebb4fd..a346aa1f653 100644
--- a/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java
+++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/CreateAliasCmd.java
@@ -29,6 +29,7 @@ import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.Aliases;
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.ZkNodeProps;
import org.apache.solr.common.cloud.ZkStateReader;
@@ -55,6 +56,7 @@ public class CreateAliasCmd extends AliasCmd {
final String aliasName = message.getStr(CommonParams.NAME);
ZkStateReader zkStateReader = ccc.getZkStateReader();
// make sure we have the latest version of existing aliases
+ //noinspection ConstantConditions
if (zkStateReader.aliasesManager != null) { // not a mock ZkStateReader
zkStateReader.aliasesManager.update();
}
@@ -72,12 +74,13 @@ public class CreateAliasCmd extends AliasCmd {
// Solr's view of the cluster is eventually consistent. *Eventually* all nodes and
// CloudSolrClients will be aware of alias changes, but not immediately. If a newly created
// alias is queried, things should work right away since Solr will attempt to see if it needs to
- // get the latest aliases when it can't otherwise resolve the name. However modifications to an
- // alias will take some time.
+ // get the latest aliases when it can't otherwise resolve the name. However, modifications to
+ // an alias will take some time.
//
- // We could levy this requirement on the client but they would probably always add an obligatory
- // sleep, which is just kicking the can down the road. Perhaps ideally at this juncture here we
- // could somehow wait until all Solr nodes in the cluster have the latest aliases?
+ // We could levy this requirement on the client, but they would probably always add an
+ // obligatory sleep, which is just kicking the can down the road. Perhaps ideally at this
+ // juncture here we could somehow wait until all Solr nodes in the cluster have the
+ // latest aliases?
Thread.sleep(100);
}
@@ -100,10 +103,10 @@ public class CreateAliasCmd extends AliasCmd {
* "b"]). We also maintain support for the legacy format, a comma-separated list (e.g. a,b).
*/
@SuppressWarnings("unchecked")
- private List<String> parseCollectionsParameter(Object colls) {
- if (colls == null) throw new SolrException(BAD_REQUEST, "missing collections param");
- if (colls instanceof List) return (List<String>) colls;
- return StrUtils.splitSmart(colls.toString(), ",", true).stream()
+ private List<String> parseCollectionsParameter(Object collections) {
+ if (collections == null) throw new SolrException(BAD_REQUEST, "missing collections param");
+ if (collections instanceof List) return (List<String>) collections;
+ return StrUtils.splitSmart(collections.toString(), ",", true).stream()
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
@@ -140,15 +143,22 @@ public class CreateAliasCmd extends AliasCmd {
Sets.difference(routedAlias.getRequiredParams(), props.keySet()), ','));
}
- // Create the first collection.
- String initialColl = routedAlias.computeInitialCollectionName();
- ensureAliasCollection(
- aliasName, zkStateReader, state, routedAlias.getAliasMetadata(), initialColl);
+ Aliases aliases = zkStateReader.aliasesManager.getAliases();
+
+ final String collectionListStr;
+ if (!aliases.isRoutedAlias(aliasName)) {
+ // Create the first collection. Prior validation ensures that this is not a standard alias
+ collectionListStr = routedAlias.computeInitialCollectionName();
+ ensureAliasCollection(
+ aliasName, zkStateReader, state, routedAlias.getAliasMetadata(), collectionListStr);
+ } else {
+ List<String> collectionList = aliases.resolveAliases(aliasName);
+ collectionListStr = String.join(",", collectionList);
+ }
// Create/update the alias
zkStateReader.aliasesManager.applyModificationAndExportToZk(
- aliases ->
- aliases
- .cloneWithCollectionAlias(aliasName, initialColl)
+ a ->
+ a.cloneWithCollectionAlias(aliasName, collectionListStr)
.cloneWithCollectionAliasProperties(aliasName, routedAlias.getAliasMetadata()));
}
diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java
index b04f824597d..b32f8980faa 100644
--- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java
+++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java
@@ -79,6 +79,7 @@ import org.apache.solr.cluster.placement.impl.PlacementPluginFactoryLoader;
import org.apache.solr.common.AlreadyClosedException;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.cloud.Aliases;
import org.apache.solr.common.cloud.DocCollection;
import org.apache.solr.common.cloud.Replica;
import org.apache.solr.common.cloud.Replica.State;
@@ -2299,6 +2300,25 @@ public class CoreContainer {
return status;
}
+ /**
+ * Retrieve the aliases from zookeeper. This is typically cached and does not hit zookeeper after
+ * the first use.
+ *
+ * @return an immutable instance of {@code Aliases} accurate as of at the time this method is
+ * invoked, less any zookeeper update lag.
+ * @throws RuntimeException if invoked on a {@code CoreContainer} where {@link
+ * #isZooKeeperAware()} returns false
+ */
+ public Aliases getAliases() {
+ if (isZooKeeperAware()) {
+ return getZkController().getZkStateReader().getAliases();
+ } else {
+ // fail fast because it's programmer error, but give slightly more info than NPE.
+ throw new IllegalStateException(
+ "Aliases don't exist in a non-cloud context, check isZookeeperAware() before calling this method.");
+ }
+ }
+
// Occasionally we need to access the transient cache handler in places other than coreContainer.
public TransientSolrCoreCache getTransientCache() {
return solrCores.getTransientCacheHandler();
diff --git a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
index dd0881295ca..cab32779b7c 100644
--- a/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
+++ b/solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
@@ -599,12 +599,7 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission
SYNCSHARD,
(req, rsp, h) -> {
String extCollection = req.getParams().required().get("collection");
- String collection =
- h.coreContainer
- .getZkController()
- .getZkStateReader()
- .getAliases()
- .resolveSimpleAlias(extCollection);
+ String collection = h.coreContainer.getAliases().resolveSimpleAlias(extCollection);
String shard = req.getParams().required().get("shard");
ClusterState clusterState = h.coreContainer.getZkController().getClusterState();
@@ -669,6 +664,17 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission
//////////////////////////////////////
return copy(finalParams.required(), null, NAME, "collections");
}
+ } else {
+ if (routedAlias != null) {
+ CoreContainer coreContainer1 = h.getCoreContainer();
+ Aliases aliases = coreContainer1.getAliases();
+ String aliasName = routedAlias.getAliasName();
+ if (aliases.hasAlias(aliasName) && !aliases.isRoutedAlias(aliasName)) {
+ throw new SolrException(
+ BAD_REQUEST,
+ "Cannot add routing parameters to existing non-routed Alias: " + aliasName);
+ }
+ }
}
/////////////////////////////////////////////////
@@ -929,12 +935,7 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission
COLLECTIONPROP,
(req, rsp, h) -> {
String extCollection = req.getParams().required().get(NAME);
- String collection =
- h.coreContainer
- .getZkController()
- .getZkStateReader()
- .getAliases()
- .resolveSimpleAlias(extCollection);
+ String collection = h.coreContainer.getAliases().resolveSimpleAlias(extCollection);
String name = req.getParams().required().get(PROPERTY_NAME);
String val = req.getParams().get(PROPERTY_VALUE);
CollectionProperties cp =
@@ -1356,11 +1357,7 @@ public class CollectionsHandler extends RequestHandlerBase implements Permission
final String collectionName =
SolrIdentifierValidator.validateCollectionName(req.getParams().get(COLLECTION_PROP));
- if (h.coreContainer
- .getZkController()
- .getZkStateReader()
- .getAliases()
- .hasAlias(collectionName)) {
+ if (h.coreContainer.getAliases().hasAlias(collectionName)) {
throw new SolrException(
ErrorCode.BAD_REQUEST,
"Collection '" + collectionName + "' is an existing alias, no action taken.");
diff --git a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
index 50b0527f1f4..3de3d919e04 100644
--- a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
+++ b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
@@ -221,12 +221,6 @@ public class HttpSolrCall {
return queryParams;
}
- protected Aliases getAliases() {
- return cores.isZooKeeperAware()
- ? cores.getZkController().getZkStateReader().getAliases()
- : Aliases.EMPTY;
- }
-
/** The collection(s) referenced in this request. Populated in {@link #init()}. Not null. */
public List<String> getCollectionsList() {
return collectionsList != null ? collectionsList : Collections.emptyList();
@@ -416,7 +410,7 @@ public class HttpSolrCall {
}
List<String> result = null;
LinkedHashSet<String> uniqueList = null;
- Aliases aliases = getAliases();
+ Aliases aliases = cores.getAliases();
List<String> inputCollections = StrUtils.splitSmart(collectionStr, ",", true);
if (inputCollections.size() > 1) {
uniqueList = new LinkedHashSet<>();
diff --git a/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java b/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
index 7f3a8722fb2..b5cb4127609 100644
--- a/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
@@ -20,9 +20,11 @@ package org.apache.solr.cloud;
import static org.apache.solr.client.solrj.RoutedAliasTypes.TIME;
import java.io.IOException;
+import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Date;
+import java.util.List;
import java.util.Map;
import java.util.TimeZone;
import org.apache.http.client.methods.CloseableHttpResponse;
@@ -39,6 +41,7 @@ import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.impl.CloudLegacySolrClient;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.cloud.api.collections.TimeRoutedAlias;
import org.apache.solr.common.cloud.Aliases;
import org.apache.solr.common.cloud.CompositeIdRouter;
@@ -189,25 +192,8 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
@Test
public void testV1() throws Exception {
final String aliasName = getSaferTestName();
- final String baseUrl = cluster.getRandomJetty(random()).getBaseUrl().toString();
Instant start = Instant.now().truncatedTo(ChronoUnit.HOURS); // mostly make sure no millis
- HttpGet get =
- new HttpGet(
- baseUrl
- + "/admin/collections?action=CREATEALIAS"
- + "&wt=xml"
- + "&name="
- + aliasName
- + "&router.field=evt_dt"
- + "&router.name=time"
- + "&router.start="
- + start
- + "&router.interval=%2B30MINUTE"
- + "&create-collection.collection.configName=_default"
- + "&create-collection.router.field=foo_s"
- + "&create-collection.numShards=1"
- + "&create-collection.replicationFactor=2");
- assertSuccess(get);
+ createTRAv1(aliasName, start);
String initialCollectionName =
TimeRoutedAlias.formatCollectionNameFromInstant(aliasName, start);
@@ -231,7 +217,104 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
assertNotNull(meta);
assertEquals("evt_dt", meta.get("router.field"));
assertEquals("_default", meta.get("create-collection.collection.configName"));
- assertEquals(null, meta.get("start"));
+ assertNull(meta.get("start"));
+ }
+
+ @Test
+ public void testUpdateRoudetedAliasDoesNotChangeCollectionList() throws Exception {
+
+ final String aliasName = getSaferTestName();
+ Instant start = Instant.now().truncatedTo(ChronoUnit.HOURS); // mostly make sure no millis
+ createTRAv1(aliasName, start);
+
+ String initialCollectionName =
+ TimeRoutedAlias.formatCollectionNameFromInstant(aliasName, start);
+ assertCollectionExists(initialCollectionName);
+
+ // Note that this is convenient for the test because it implies a different collection name, but
+ // doing this is an advanced operation, typically preceded by manual collection creations and
+ // manual tweaking of the collection list. This is here merely to test that we don't blow away
+ // the existing (possibly tweaked) list. DO NOT use this as an example of normal operations.
+ Instant earlierStart = start.minus(Duration.ofMinutes(3));
+ createTRAv1(aliasName, earlierStart);
+ assertCollectionExists(initialCollectionName);
+
+ // Test Alias metadata
+ Aliases aliases = cluster.getZkStateReader().getAliases();
+ Map<String, String> collectionAliasMap = aliases.getCollectionAliasMap();
+ String alias = collectionAliasMap.get(aliasName);
+ assertNotNull(alias);
+ Map<String, String> meta = aliases.getCollectionAliasProperties(aliasName);
+ assertNotNull(meta);
+ assertEquals("evt_dt", meta.get("router.field"));
+ assertEquals("_default", meta.get("create-collection.collection.configName"));
+
+ // This should be equal to the new start value
+ assertEquals(earlierStart.toString(), meta.get("router.start"));
+ List<String> collectionList = aliases.resolveAliases(aliasName);
+ assertEquals(1, collectionList.size());
+ assertTrue(collectionList.contains(initialCollectionName));
+ }
+
+ public void testCantAddRoutingToNonRouted() throws Exception {
+ String aliasName = getSaferTestName() + "Alias";
+ createCollection();
+ final String baseUrl = cluster.getRandomJetty(random()).getBaseUrl().toString();
+ HttpGet get =
+ new HttpGet(
+ baseUrl
+ + "/admin/collections?action=CREATEALIAS"
+ + "&wt=xml"
+ + "&name="
+ + aliasName
+ + "&collections="
+ + getSaferTestName());
+ assertSuccess(get);
+
+ HttpGet get2 =
+ new HttpGet(
+ baseUrl
+ + "/admin/collections?action=CREATEALIAS"
+ + "&wt=json"
+ + "&name="
+ + aliasName
+ + "&router.field=evt_dt"
+ + "&router.name=time"
+ + "&router.start=2018-01-15T00:00:00Z"
+ + "&router.interval=%2B30MINUTE"
+ + "&create-collection.collection.configName=_default"
+ + "&create-collection.numShards=1");
+ assertFailure(get2, "Cannot add routing parameters to existing non-routed Alias");
+ }
+
+ private void createCollection() throws SolrServerException, IOException {
+ final CollectionAdminResponse response =
+ CollectionAdminRequest.createCollection(getSaferTestName(), "_default", 1, 1)
+ .process(solrClient);
+ if (response.getStatus() != 0) {
+ fail("failed to create collection " + getSaferTestName());
+ }
+ }
+
+ private void createTRAv1(String aliasName, Instant start) throws IOException {
+ final String baseUrl = cluster.getRandomJetty(random()).getBaseUrl().toString();
+ HttpGet get =
+ new HttpGet(
+ baseUrl
+ + "/admin/collections?action=CREATEALIAS"
+ + "&wt=xml"
+ + "&name="
+ + aliasName
+ + "&router.field=evt_dt"
+ + "&router.name=time"
+ + "&router.start="
+ + start
+ + "&router.interval=%2B30MINUTE"
+ + "&create-collection.collection.configName=_default"
+ + "&create-collection.router.field=foo_s"
+ + "&create-collection.numShards=1"
+ + "&create-collection.replicationFactor=2");
+ assertSuccess(get);
}
// TZ should not affect the first collection name if absolute date given for start
diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/alias-management.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/alias-management.adoc
index 15fdc27912e..c01bf492bd3 100644
--- a/solr/solr-ref-guide/modules/deployment-guide/pages/alias-management.adoc
+++ b/solr/solr-ref-guide/modules/deployment-guide/pages/alias-management.adoc
@@ -111,13 +111,15 @@ If routing parameters are present this parameter is prohibited.
==== Routed Alias Parameters
-Most routed alias parameters become _alias properties_ that can subsequently be inspected and <<aliasprop,modified>>.
+Most routed alias parameters become _alias properties_ that can subsequently be inspected and modified either by issuing a new CREATEALIAS for the same name or via <<aliasprop,ALIASPROP>>.
+CREATEALIAS will validate against many (but not all) bad values, whereas ALIASPROP blindly accepts any key or value you give it.
+Some "valid" modifications allowed by CREATEALIAS may still be unwise, see notes below. "Expert only" modifications are technically possible, but require good understanding of how the code works and may require several precursor operations.
`router.name`::
+
[%autowidth,frame=none]
|===
-s|Required |Default: none
+s|Required |Default: none |Modify: Do not change after creation
|===
+
The type of routing to use.
@@ -126,15 +128,17 @@ Presently only `time` and `category` and `Dimensional[]` are valid.
In the case of a xref:aliases.adoc#dimensional-routed-aliases[multi-dimensional routed alias] (aka "DRA"), it is required to express all the dimensions in the same order that they will appear in the dimension
array.
The format for a DRA `router.name` is `Dimensional[dim1,dim2]` where `dim1` and `dim2` are valid `router.name` values for each sub-dimension.
-Note that DRA's are very new, and only 2D DRA's are presently supported.
-Higher numbers of dimensions will be supported soon.
+Note that DRA's are experimental, and only 2D DRA's are presently supported.
+Higher numbers of dimensions may be supported in the future.
+Careful design of dimensional routing is required to avoid an explosion in the number of collections in the cluster.
+Solr Cloud may have difficulty managing more than a thousand collections.
See examples below for further clarification on how to configure individual dimensions.
`router.field`::
+
[%autowidth,frame=none]
|===
-s|Required |Default: none
+s|Required |Default: none |Modify: Do not change after creation
|===
+
The field to inspect to determine which underlying collection an incoming document should be routed to.
@@ -144,7 +148,7 @@ This field is required on all incoming documents.
+
[%autowidth,frame=none]
|===
-|Optional |Default: none
+|Optional |Default: none | Modify: Yes, only new collections affected, use with care
|===
+
The `*` wildcard can be replaced with any parameter from the xref:collection-management.adoc#create[CREATE] command except `name`.
@@ -158,7 +162,7 @@ It's probably a bad idea to use "data driven" mode as schema mutations might hap
+
[%autowidth,frame=none]
|===
-s|Required |Default: none
+s|Required |Default: none | Modify: Expert only
|===
+
The start date/time of data for this time routed alias in Solr's standard date/time format (i.e., ISO-8601 or "NOW" optionally with xref:indexing-guide:date-formatting-math.adoc#date-math[date math]).
@@ -172,7 +176,7 @@ Particularly, this means `NOW` will fail 999 times out of 1000, though `NOW/SECO
+
[%autowidth,frame=none]
|===
-|Optional |Default: `UTC`
+|Optional |Default: `UTC` | Modify: Expert only
|===
+
The timezone to be used when evaluating any date math in `router.start` or `router.interval`.
@@ -186,7 +190,7 @@ If GMT-4 is supplied for this value then a document dated 2018-01-14T21:00:00:01
+
[%autowidth,frame=none]
|===
-s|Required |Default: none
+s|Required |Default: none | Modify: Yes
|===
+
A date math expression that will be appended to a timestamp to determine the next collection in the series.
@@ -196,7 +200,7 @@ Any date math expression that can be evaluated if appended to a timestamp of the
+
[%autowidth,frame=none]
|===
-|Optional |Default: `600000` milliseconds
+|Optional |Default: `600000` milliseconds | Modify: Yes
|===
+
The maximum milliseconds into the future that a document is allowed to have in `router.field` for it to be accepted without error.
@@ -206,7 +210,7 @@ If there was no limit, then an erroneous value could trigger many collections to
+
[%autowidth,frame=none]
|===
-|Optional |Default: none
+|Optional |Default: none | Modify: Yes
|===
+
A date math expression that results in early creation of new collections.
@@ -233,7 +237,7 @@ This property is empty by default indicating just-in-time, synchronous creation
+
[%autowidth,frame=none]
|===
-|Optional |Default: none
+|Optional |Default: none | Modify: Yes, Possible data loss, use with care!
|===
+
A date math expression that results in the oldest collections getting deleted automatically.
@@ -251,7 +255,7 @@ The default is not to delete.
+
[%autowidth,frame=none]
|===
-|Optional |Default: none
+|Optional |Default: none | Modify: Yes
|===
+
The maximum number of categories allowed for this alias.
@@ -261,7 +265,7 @@ This setting safeguards against the inadvertent creation of an infinite number o
+
[%autowidth,frame=none]
|===
-|Optional |Default: none
+|Optional |Default: none | Modify: Yes
|===
+
A regular expression that the value of the field specified by `router.field` must match before a corresponding collection will be created.
@@ -277,7 +281,7 @@ Overly complex patterns will produce CPU or garbage collection overhead during i
+
[%autowidth,frame=none]
|===
-|Optional |Default: none
+|Optional |Default: none | Modify: As per above
|===
+
This prefix denotes which position in the dimension array is being referred to for purposes of dimension configuration.