[GitHub] [lucene-solr] sigram commented on a change in pull request #1716: SOLR-14706: Fix support for default autoscaling policy

[GitBox <gi...@apache.org>]

sigram commented on a change in pull request #1716:
URL: https://github.com/apache/lucene-solr/pull/1716#discussion_r466605349



##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Clause.java
##########
@@ -117,10 +117,10 @@ private Clause(Map<String, Object> m) {
     strict = Boolean.parseBoolean(String.valueOf(m.getOrDefault("strict", "true")));
     Optional<String> globalTagName = m.keySet().stream().filter(Policy.GLOBAL_ONLY_TAGS::contains).findFirst();
     if (globalTagName.isPresent()) {
-      globalTag = parse(globalTagName.get(), m);
-      if (m.size() > 2) {
-        throw new RuntimeException("Only one extra tag supported for the tag " + globalTagName.get() + " in " + toJSONString(m));
+      if (m.size() > 3) {

Review comment:
       @gus-asf yes, that's the intent, though your pseudo-code is still incorrect - if there's no `strict` tag then `m.size() > 2` is already an error. In other words, for global tags we expect exactly 2 keys (tag and operand), with optional third key `strict`.

##########
File path: solr/solr-ref-guide/src/solr-upgrade-notes.adoc
##########
@@ -72,7 +84,9 @@ More information about this new feature is available in the section <<common-que
 
 *Autoscaling*
 
-* Solr now includes a default autoscaling policy.
+* **NOTE: The default autoscaling policy has been removed as of 8.6.1**
++
+Solr now includes a default autoscaling policy.

Review comment:
       This statement conflicts with the statement above. 8.5.x and 8.6.1 includes default preferences but not the default policy.

##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimScenario.java
##########
@@ -814,7 +814,9 @@ public void execute(SimScenario scenario) throws Exception {
         values.put(key, val);
       }
       for (String node : nodes) {
-        scenario.cluster.getSimNodeStateProvider().simSetNodeValues(node, values);
+        Map<String, Object> newValues = new HashMap<>(scenario.cluster.getSimNodeStateProvider().simGetNodeValues(node));

Review comment:
       +1, this fixed a genuine bug in < 8.6.0

##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Clause.java
##########
@@ -686,7 +686,7 @@ boolean isShardAbsent() {
       for (Row r : session.matrix) {
         computedValueEvaluator.node = r.node;
         SealedClause sealedClause = getSealedClause(computedValueEvaluator);
-        if (!sealedClause.getGlobalTag().isPass(r)) {
+        if (r.isLive() && !sealedClause.getGlobalTag().isPass(r)) {

Review comment:
       +1, this was a genuine bug in 8.5.

##########
File path: solr/solr-ref-guide/src/solr-upgrade-notes.adoc
##########
@@ -34,17 +34,29 @@ Detailed steps for upgrading a Solr cluster are in the section <<upgrading-a-sol
 
 If you are upgrading from 7.x, see the section <<Upgrading from 7.x Releases>> below.
 
-=== Solr 8.6.1
+=== Solr 8.6.1 (Upgrading from 8.6.0 only)
+
+See the https://cwiki.apache.org/confluence/display/SOLR/ReleaseNote861[8.6.1 Release Notes^]
+for an overview of the fixes included in Solr 8.6.1.
+
+When upgrading to 8.6.1 users should be aware of the following major changes from 8.6.0.
 
 *Autoscaling*
 
 * As mentioned in the 8.6 upgrade notes, a default autoscaling policy was provided starting in 8.6.0.
 This default autoscaling policy resulted in increasingly slow collection creation calls in large clusters (50+ collections).
 +
 In 8.6.1 the default autoscaling policy has been removed, and clouds will not use autoscaling unless a policy has explicitly been created.
-In order to fix the performance degradations introduced in 8.6.0, merely upgrade to 8.6.1.
+If your cloud is running 8.6.0 and **not using an explicit autoscaling policy**, upgrade to 8.6.1 and remove the default cluster policy and preferences via the following command.
+Replace `localhost:8983` with your Solr endpoint.
++
+```
+curl -X POST -H 'Content-type:application/json'  -d '{set-cluster-policy : [], set-cluster-preferences : []}' http://localhost:8983/api/cluster/autoscaling

Review comment:
       Setting preferences to `[]` actually removes the default preferences that have always been implicitly present, so this changes the behavior as compared with 8.5 and earlier. We should only reset the cluster_policy to `[]`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org