Solr
  1. Solr
  2. SOLR-7180

MiniSolrCloudCluster should startup and shutdown its jetties in parallel

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Followup to SOLR-7179. Now JettySolrRunner doesn't use sysprops to pass configuration, we can start up multiple runners in parallel.

      1. SOLR-7180.patch
        20 kB
        Alan Woodward
      2. SOLR-7180.patch
        3 kB
        Alan Woodward

        Activity

        Hide
        Alan Woodward added a comment -

        Patch. With this change, MiniSolrCloudClusterTest went from taking 26s to taking 14s on my laptop.

        Show
        Alan Woodward added a comment - Patch. With this change, MiniSolrCloudClusterTest went from taking 26s to taking 14s on my laptop.
        Hide
        Vamsee Yarlagadda added a comment -

        LGTM. One small concern.

        executor.invokeAll(startups);
        executor.invokeAll(shutdowns);

        How do we know whether all the jetty runners came up/shutdown properly? invokeAll will return once the Future.isDone() on every task right. But it doesn't guarantee that there are no exceptions in the process.

        Show
        Vamsee Yarlagadda added a comment - LGTM. One small concern. executor.invokeAll(startups); executor.invokeAll(shutdowns); How do we know whether all the jetty runners came up/shutdown properly? invokeAll will return once the Future.isDone() on every task right. But it doesn't guarantee that there are no exceptions in the process.
        Hide
        Tomás Fernández Löbbe added a comment -

        Should we awaitTermination in shutdown too? Also, is there a reason why you don't call "stopJettySolrRunner"?

        Show
        Tomás Fernández Löbbe added a comment - Should we awaitTermination in shutdown too? Also, is there a reason why you don't call "stopJettySolrRunner"?
        Hide
        Alan Woodward added a comment -

        Calling awaitTermination shouldn't be necessary, because invokeAll() won't return until everything has finished, but it won't hurt anything and is a good check to have. I'll add it in.

        I didn't use stopJettySolrRunner() because it relies on the position of a Jetty in the jettys list, which will not be constant if we're calling it in parallel. It's probably worth clearing the list after everything's done though.

        Error handling is a slightly tricky one. Should we fail startup if one of the jetties doesn't start? The behaviour at the moment isn't great - you'll get an exception if one of the starting jetties dies, but the already-started-up ones will keep running. At close, I guess the nicest behaviour would be to collect all exceptions from the futures, and throw an IOException with any collected exceptions added as suppressed.

        Show
        Alan Woodward added a comment - Calling awaitTermination shouldn't be necessary, because invokeAll() won't return until everything has finished, but it won't hurt anything and is a good check to have. I'll add it in. I didn't use stopJettySolrRunner() because it relies on the position of a Jetty in the jettys list, which will not be constant if we're calling it in parallel. It's probably worth clearing the list after everything's done though. Error handling is a slightly tricky one. Should we fail startup if one of the jetties doesn't start? The behaviour at the moment isn't great - you'll get an exception if one of the starting jetties dies, but the already-started-up ones will keep running. At close, I guess the nicest behaviour would be to collect all exceptions from the futures, and throw an IOException with any collected exceptions added as suppressed.
        Hide
        Alan Woodward added a comment -

        Patch with some error handling.

        Show
        Alan Woodward added a comment - Patch with some error handling.
        Hide
        Alan Woodward added a comment -

        Thanks all!

        Show
        Alan Woodward added a comment - Thanks all!
        Hide
        ASF subversion and git services added a comment -

        Commit 1665353 from Alan Woodward in branch 'dev/trunk'
        [ https://svn.apache.org/r1665353 ]

        SOLR-7180: MiniSolrCloudCluster starts up and shuts down its jetties in parallel

        Show
        ASF subversion and git services added a comment - Commit 1665353 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1665353 ] SOLR-7180 : MiniSolrCloudCluster starts up and shuts down its jetties in parallel
        Hide
        ASF subversion and git services added a comment -

        Commit 1665355 from Alan Woodward in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1665355 ]

        SOLR-7180: MiniSolrCloudCluster starts up and shuts down its jetties in parallel

        Show
        ASF subversion and git services added a comment - Commit 1665355 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1665355 ] SOLR-7180 : MiniSolrCloudCluster starts up and shuts down its jetties in parallel

          People

          • Assignee:
            Alan Woodward
            Reporter:
            Alan Woodward
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development