You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/10/19 12:18:10 UTC
[GitHub] [solr] janhoy opened a new pull request, #1089: SOLR-16475 Make the default replica placement plugin configurable
janhoy opened a new pull request, #1089:
URL: https://github.com/apache/solr/pull/1089
...as a system property or env variable
https://issues.apache.org/jira/browse/SOLR-16475
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1000643366
##########
solr/packaging/test/bats_helper.bash:
##########
@@ -20,7 +20,7 @@
# The SOLR_HOME directory will be cleared when the next test file is executed.
# - "setup" should use "common_setup" if a Solr process is NOT being started in that same "setup" function.
common_setup() {
- bats_require_minimum_version 1.8.0
+ bats_require_minimum_version 1.8.2
Review Comment:
Not sure. `bats-support` is a npm dependency of `bats-assert`, so it is still being used, perhaps it is auto loaded somewhere?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1001623566
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
Review Comment:
Well, yes. I left it cause we may want to switch the ootb default to affinity in 9.x. We could make a JIRA about affinity as default, and link to that jira in the comment instead?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1004211846
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ }
}
return new PlacementPluginAssignStrategy(placementPlugin);
Review Comment:
I parked this proposal in https://issues.apache.org/jira/browse/SOLR-16493
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r999795723
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
An example of changes required to load the bats stuff is here: https://github.com/apache/solr/pull/1091
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] murblanc commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
murblanc commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1001442865
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
Review Comment:
Why not pass the fully qualified classname in the system property so that Assign.java does not have to be touched to use a new placement plugin? Security concerns?
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
Review Comment:
This comment now goes away since you implement a better option.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ }
}
return new PlacementPluginAssignStrategy(placementPlugin);
Review Comment:
I'm afraid things might get a bit confusing when multiple configurations collide.
There's the default default plugin (Simple), then there's an override to the default (introduced in this PR) and then there's a ZK configured plugin.
It likely makes no practical sense to both have a ZK configured plugin and a default override at the same time. Should we warn here if that's the case?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1000217240
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
Nice, I struggled a bit with installing the correct version of bats-file, but once I settled on the latest versions from the https://github.com/bats-core repo (i.e. upgraded to bats 1.8.2), all was good.
Removed sleep after start with an undocumented solr command which will retry for a configurable time:
```bash
solr assert -c http://localhost:8983/solr -t 3000
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r999757597
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
might be worth looking at adding https://github.com/bats-core/bats-file - which has https://github.com/bats-core/bats-file#assert_file_contains
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1000248321
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
@risdenk The `bats` package in NPM is the correct one, but `ztombol/bats-support#v0.2.0`, `ztombol/bats-assert#v0.3.0` are old versions from old repo, and the `ztombol/bats-file#v0.2.0` does not have the `assert_file_contains` method. Thus I pinned all of these on the latest from `bats-core` github repo.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1290308738
> > Hmm, I wonder if I prefer `solr.placementplugin.default` and `SOLR_PLACEMENTPLUGIN_DEFAULT` to keep with the dot separated lowercased convention. And that the env.var is the same in uppercase and underscore. Wdyt?
>
> I'm good either way
I switched to `solr.placementplugin.default`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1000217240
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
Nice, I struggled a bit with installing the correct version of bats-file, but once I settled on the latest versions from the https://github.com/bats-core repo, all was good.
Removed sleep after start with an undocumented solr command which will retry for a configurable time:
```bash
solr assert -c http://localhost:8983/solr -t 3000
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1285876102
Hmm, I wonder if I prefer `solr.placementplugin.default` and `SOLR_PLACEMENTPLUGIN_DEFAULT` to keep with the dot separated lowercased convention. And that the env.var is the same in uppercase and underscore. Wdyt?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r999584109
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,33 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId) {
Review Comment:
Done
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1290310966
I'm ready to merge this soon..
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1001622215
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
Review Comment:
If we have SolrResourceLoader available that should not be too hard, unless you also need to support loading plugins from solr packages, which (unfortunately) uses a custom loader system with need for package prefixes.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] murblanc commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
murblanc commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1286569184
I like the change I think it goes in the right direction in an elegant way. Thank you.
Should we be more ambitious and allow complete plugin setup via command line? This would require being able to specify not only the plugin name/type but also plugin configuration parameters in case (most of the time?) the hard coded values in the plugin code are not appropriate.
Short of that, ZK configuration updates are still be required in order to use a placement plugin "for real", and the benefit of overriding the default via command lines is reduced.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1000592815
##########
solr/packaging/test/bats_helper.bash:
##########
@@ -20,7 +20,7 @@
# The SOLR_HOME directory will be cleared when the next test file is executed.
# - "setup" should use "common_setup" if a Solr process is NOT being started in that same "setup" function.
common_setup() {
- bats_require_minimum_version 1.8.0
+ bats_require_minimum_version 1.8.2
Review Comment:
Is this line below still needed?
`load "${BATS_LIB_PREFIX}/bats-support/load.bash"`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1004215082
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
Review Comment:
Tweaked the TODO, linking to a new JIRA https://issues.apache.org/jira/browse/SOLR-16492
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1004214026
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
Review Comment:
Keeping it simple in this first iteration, postponing class loading until someone asks for it :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1285909573
> Hmm, I wonder if I prefer `solr.placementplugin.default` and `SOLR_PLACEMENTPLUGIN_DEFAULT` to keep with the dot separated lowercased convention. And that the env.var is the same in uppercase and underscore. Wdyt?
I'm good either way
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
madrob commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r999818589
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
Having to sleep is a symptom of us not supporting a collection ready API, but bats-file should do the cat part for you.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1063021252
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2841,8 +2836,10 @@ static CollectionAdminResponse getStatusResponse(String requestId, SolrClient cl
}
protected void setupRestTestHarnesses() {
- for (final SolrClient client : clients) {
- RestTestHarness harness = new RestTestHarness(() -> ((HttpSolrClient) client).getBaseURL());
+ for (final JettySolrRunner jetty : jettys) {
+ RestTestHarness harness =
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1342933419
> I wonder why the BATS test keeps failing?
Not sure - I created https://github.com/apache/solr/pull/1223 which just changes the github actions file. This is to try to eliminate your changes as the cause.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r999563220
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,33 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId) {
Review Comment:
Can you do a `.toLowercase()` here? Thinking about ease-of-use for the `minimizeCores` option.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1063021233
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2841,8 +2836,10 @@ static CollectionAdminResponse getStatusResponse(String requestId, SolrClient cl
}
protected void setupRestTestHarnesses() {
- for (final SolrClient client : clients) {
- RestTestHarness harness = new RestTestHarness(() -> ((HttpSolrClient) client).getBaseURL());
+ for (final JettySolrRunner jetty : jettys) {
+ RestTestHarness harness =
Review Comment:
@sonatype-lift ignore
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy merged pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy merged PR #1089:
URL: https://github.com/apache/solr/pull/1089
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1286744238
> I like the change I think it goes in the right direction in an elegant way. Thank you.
>
> Should we be more ambitious and allow complete plugin setup via command line? This would require being able to specify not only the plugin name/type but also plugin configuration parameters in case (most of the time?) the hard coded values in the plugin code are not appropriate. Short of that, ZK configuration updates are still be required in order to use a placement plugin "for real", and the benefit of overriding the default via command lines is reduced. Not necessarily in this PR but maybe as a follow up?
I have thought of that and experimented with some code that would allow you to do as follows:
```bash
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1001629307
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ }
}
return new PlacementPluginAssignStrategy(placementPlugin);
Review Comment:
The keyword here is "default". This mechanism is only to set the default plugin. Say you are a platform owner provisioning a Solr cluster for your teams. You set all kind of defaults, like RAM etc, and one of those settings would be the placement policy. Once the teams start using the cluster, they may reconfigure Solr in any way that fits them, thus overriding the pre-packaged defaults.
If we had another mechanism for statically provisioning `/clusterprops.json` we'd likely not have this discussion. Imagine Solr looking for a `$SOLR_HOME/bootstrap/clusterprops.json` at startup, and uploading it to ZK if and only if the file in ZK is empty/blank.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r999373111
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
Is there a smarter way to check Solr's log than waiting a few secs and cat'ing it? :) I don't like sleep in tests
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1000594008
##########
solr/packaging/test/test_placement_plugin.bats:
##########
@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load bats_helper
+
+setup() {
+ common_clean_setup
+}
+
+teardown() {
+ # save a snapshot of SOLR_HOME for failed tests
+ save_home_on_failure
+
+ delete_all_collections
+ solr stop -all >/dev/null 2>&1
+}
+
+@test "Affinity placement plugin using sysprop" {
+ run solr start -c -Dsolr.defaultPlacementPlugin=affinity
+ sleep 3
+ run solr create_collection -c COLL_NAME
+ sleep 3
+ run cat ${SOLR_HOME}/logs/solr.log
Review Comment:
Nice I couldn't figure out why bats-core stuff wouldn't work. Guess removing bats-support was all that is necessary
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1041162192
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2841,8 +2836,10 @@ static CollectionAdminResponse getStatusResponse(String requestId, SolrClient cl
}
protected void setupRestTestHarnesses() {
- for (final SolrClient client : clients) {
- RestTestHarness harness = new RestTestHarness(() -> ((HttpSolrClient) client).getBaseURL());
+ for (final JettySolrRunner jetty : jettys) {
+ RestTestHarness harness =
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.setupRestTestHarnesses()` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=358484317&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=358484317&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=358484317&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=358484317&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=358484317&lift_comment_rating=5) ]
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
Posted by GitBox <gi...@apache.org>.
janhoy commented on PR #1089:
URL: https://github.com/apache/solr/pull/1089#issuecomment-1342260703
I wonder why the BATS test keeps failing?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org