Port defaulting should be the same for all tools #1951

HoustonPutman · 2023-09-22T22:06:21Z

If Solr processes need to be found, port scanning will always be used.

I need to make a JIRA for this, but this has been exhausting, so it can wait till later.

Basically any Solr tool accepts -p, -host and -urlScheme, and can default the Solr URL from that. If a port is not provided, it will use SOLR_PORT if that is explicitly set beforehand. Otherwise, it will scan processes for a Solr Port. If more than one process is found, it will fail.

Similarly the tools (stop -all, status) that find all running Solr instances will use port scanning instead of PIDs.

This is somewhat back-incompat, for tools that use to use port scanning instead of SOLR_PORT, but I think this is acceptable. This can also wait for 10.0 if we want to, but I don't think that's necessary.

This is a WIP and likely needs more tests, and possibly there are some tools that I forgot.

If Solr processes need to be found, port scanning will always be used.

janhoy · 2023-09-23T12:11:24Z

Wow, huge effort. Love consistency! Will this need changes in solr.cmd as well? 🫣
Will try to circle back to this and take it for a spin. It's great having the bats tests to guard against regressions here!

epugh · 2023-09-23T13:03:01Z

I absolutely love all this clean up! I'll be honest, all the flavours of variables have been a bit confusing ;-)... I think some accretion over time of concepts layered on concepts...

So one thing I'll ask, "Is any of this required?"... I suspect that I don't quite understand how some folks use Solr... I pretty much only ever run one Solr per VM/image with proxies in front etc, so in my world, Solr is always http, Solr is always localhost (127.0.01?), and Solr is always port 8983 when I am on that node. I had actually been hoping to move in the direction of eliminating the whole port, schema, host variables, and just using a url everywhere.... I want to encourage us to be able to use the various commands like bin/solr create from anywhere, which would imply that you pretty much only ever do a bin/solr [your_command] --solrUrl https://myserverincloud:50678 -u user:password instead of the current way of looking up local variables. This pattern may come from I generally stand up a solr as step one, and then externally, as step 2, interact with it.

Regardless, this is all amazing work!

epugh · 2023-09-23T13:03:55Z

solr/bin/solr

@@ -148,7 +148,7 @@ fi
 : "${SOLR_START_WAIT:=$SOLR_STOP_WAIT}" # defaulting to $SOLR_STOP_WAIT for backwards compatibility reasons

 # Store whether a solr port was explicitly provided, for the "solr stop" command.
-PROVIDED_SOLR_PORT="${SOLR_PORT:-}"
+PROVIDED_SOLR_PORT_ENV="${SOLR_PORT:-}"


is _ENV a pattern we use elsewhere in bin/solr? For these variables? If we have similar concepts, maybe we can get all of the variables following this pattern?

epugh · 2023-09-23T13:05:21Z

solr/bin/solr

@@ -626,6 +626,7 @@ function jetty_port() {
 # run a Solr command-line tool using the SolrCLI class;
 # useful for doing cross-platform work from the command-line using Java
 function run_tool() {
+  export SOLR_PORT="$(choose_default_solr_port)"


would (choose_default_solr_port()) be possible to signal it's a function? just a spitball...

epugh · 2023-09-23T13:06:27Z

solr/bin/solr

-
-function run_package() {
-  runningSolrUrl=""
+function get_all_running_solr_ports() {


I love the breaking down these convulted functions into smaller ones, makes it easier to read and reason! Hell, maybe someday we bats test the functions?

epugh · 2023-09-23T13:07:26Z

solr/bin/solr

-            echo ""
-          fi
-      done
+    # no-commit - check if the SOLR_TOOL_HOST is the localhost, otherwise don't do this


I never quite understood what SOLR_TOOL_HOST gave us that SOLR_HOST doesn't!

Assuming that you are running your tool on the same node as a Solr and therefore HAVE a SOLR_HOST.

epugh · 2023-09-23T13:08:46Z

solr/bin/solr

-          else
-            exit # subshell!
-          fi
+  echo -e "Sending stop command to Solr running on port $SOLR_PORT ... waiting up to $SOLR_STOP_WAIT seconds to allow Jetty process $SOLR_PID to stop gracefully."


We talk about "Solr process" in various messages, except here, we mention Jetty... Maybe time to accept that Jetty is an implmentation detail, and change this to "Solr process" ;-)!!

epugh · 2023-09-23T13:09:20Z

solr/bin/solr

+      # Check if a process is running with the specified PID.
+      # -o stat will output the STAT, where Z indicates a zombie
+      # stat='' removes the header (--no-headers isn't supported on all platforms)
+      # Note the space after '$('. It is needed to avoid confusion with special bash eval syntax


An this is why I ❤️ bash.. ;-).

epugh · 2023-09-23T13:10:19Z

solr/bin/solr

@@ -1017,7 +991,7 @@ if [[ "$SCRIPT_CMD" == "package" ]]; then
      esac
    done
  fi
-  run_package "$@"
+  run_tool package "$@"


thank you! This has been on my backlog for a while to fix... I think you can delete run_package now too!

epugh · 2023-09-23T13:11:30Z

solr/bin/solr

-      done
-  fi
-  run_tool auth "${AUTH_PARAMS[@]}" -solrUrl "$SOLR_URL_SCHEME://$SOLR_TOOL_HOST:${AUTH_PORT:-8983}/solr" -authConfDir "$SOLR_HOME" $VERBOSE
+  run_tool auth "${AUTH_PARAMS[@]}" -authConfDir "$SOLR_HOME" $VERBOSE


You can see where I was struggling to figure out how to eliminate the SOLR_URL_SCHEME and firends, and yet it was sneaking back in. This at least is a lot shorter!

Also, I was noticing in bin/solr auth getting the warning about the solrUrl not needing /solr and this fixes it, assuming we aren't constructing a solr url internally with a trailing /solr.

epugh · 2023-09-23T13:13:40Z

solr/core/src/java/org/apache/solr/cli/ApiTool.java

@@ -56,24 +56,38 @@ public List<Option> getOptions() {
            .argName("URL")
            .hasArg()
            .required(true)
-            .desc("Send a GET request to a Solr API endpoint.")
-            .build());
+            .desc("Send a GET request to a Solr API endpoint, or path to use with the default Solr URL.")


nice tweak to the tool, in a world where you can assume your solr!

epugh · 2023-09-23T13:14:41Z

solr/core/src/java/org/apache/solr/cli/ApiTool.java

+        SolrCLI.OPTION_SOLRPORT,
+        SolrCLI.OPTION_SOLRHOST,
+        SolrCLI.OPTION_URLSCHEME,
+        SolrCLI.OPTION_VERBOSE);
  }

  @Override
  public void runImpl(CommandLine cli) throws Exception {
    String response = null;
    String getUrl = cli.getOptionValue("get");
    if (getUrl != null) {


Do we need this check? If the get parameter is marked required in the ocnfiguration, I would assume you wouldn't get to here if it is missing!

epugh · 2023-09-23T13:16:22Z

solr/core/src/java/org/apache/solr/cli/CreateTool.java

@@ -131,7 +134,7 @@ public void runImpl(CommandLine cli) throws Exception {

  protected void createCore(CommandLine cli, SolrClient solrClient) throws Exception {
    String coreName = cli.getOptionValue("name");
-    String solrUrl = cli.getOptionValue("solrUrl", SolrCLI.getDefaultSolrUrl());
+    String solrUrl = cli.getOptionValue("solrUrl", SolrCLI.getDefaultSolrUrl(cli));


Out of curiosity, if SolrCLI.getDefaultSolrUrl(cli)) always takes a cli, should we make it be cli.getDefaultSolrUrl() instead?

epugh · 2023-09-23T13:18:14Z

solr/core/src/java/org/apache/solr/cli/PackageTool.java

@@ -310,6 +310,18 @@ private Pair<String, String> parsePackageVersion(String arg) {
  public List<Option> getOptions() {
    return List.of(
        SolrCLI.OPTION_SOLRURL,
+        // Cannot use the default OPTION_SOLRPORT, because -p is already taken by PARAMS


Yeah... I wonder if we should make the slightly breaking chagne? This feels like we are going to get bit by this in the future? Or, do we think humans will never actually use teh -p parameter to mean a solr port, becuase it's a config file type thing... I could see just forcing --package, cause how often do you type this command? whereas -p is super common IF we think humans will use these parameters a lot. <-- did that make sense?

epugh · 2023-09-23T13:27:04Z

solr/core/src/java/org/apache/solr/cli/RunExampleTool.java

@@ -869,6 +857,7 @@ protected boolean isValidConfig(File configsetsDir, String config) {
  }

  protected Map<String, Object> getNodeStatus(String solrUrl, int maxWaitSecs) throws Exception {
+    solrUrl += "/solr";


bummer! But we are in an awkward place where a solrUrl can end with /solr or with /api ;-) So maybe this pattern becomes more common in varous tools... Actually, are you sure you need this? Ithought the status tool would deal with the solrURL.....

epugh · 2023-09-23T13:27:15Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

@@ -80,7 +80,7 @@ public class SolrCLI implements CLIO {
  public static final Option OPTION_ZKHOST =
      Option.builder("z")
          .longOpt("zkHost")
-          .argName("HOST")


epugh · 2023-09-23T13:29:06Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

+          .hasArg()
+          .required(false)
+          .desc(
+              "Solr URL Scheme, which will be used, along with SOLR_TOOL_HOST and SOLR_PORT, to determine the Solr URL to connect to if a solrURL is not provided; defaults to: "


SOLR_HOST instead of SOLR_TOOL_HOST? again, I don't quite get the difference ;-).

epugh · 2023-09-23T13:32:03Z

solr/packaging/test/test_create_collection.bats

+  assert_output --partial "assuming solrUrl is http://localhost:${tmp_port}"
+}
+
+@test "create collection with port scanning" {


Is port scanning really a feature we want? Do people just randomly launch solr on random port, and then scan to find it? If poeple do, then great feature, and I LOVE the test... Just kind of wondering about that use case... (Look, I used the word "use case" ;-). )

epugh · 2023-09-23T13:33:02Z

solr/packaging/test/test_packages.bats

@@ -29,7 +29,12 @@ teardown() {
 }

 @test "package detects no running solr" {
-  # not sure this is actually a good thing..  we may not want this..


I kind of want to keep this comment... There is an issue open to look at this. Why do we check if solr is running on this command?

epugh · 2023-09-23T13:42:28Z

solr/packaging/test/test_status.bats

@@ -43,7 +43,6 @@ teardown() {
 @test "status shell script ignores passed in -solrUrl cli parameter from user" {
  solr start
  run solr status -solrUrl http://localhost:9999/solr
-  assert_output --partial "needn't include Solr's context-root"


? no longer germaine? as in we're checking something that no longer needs checking because this is how it works?

Port defaulting should be the same for all tools

e93434d

If Solr processes need to be found, port scanning will always be used.

HoustonPutman requested review from epugh and janhoy September 22, 2023 22:06

epugh reviewed Sep 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port defaulting should be the same for all tools #1951

Port defaulting should be the same for all tools #1951

HoustonPutman commented Sep 22, 2023

janhoy commented Sep 23, 2023

epugh commented Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023 •

edited

Loading

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

epugh Sep 23, 2023

Port defaulting should be the same for all tools #1951

Are you sure you want to change the base?

Port defaulting should be the same for all tools #1951

Conversation

HoustonPutman commented Sep 22, 2023

janhoy commented Sep 23, 2023

epugh commented Sep 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh Sep 23, 2023 •

edited

Loading