SOLR-17118: Simplify CoreContainerProvider initialization #2474

dsmiley · 2024-05-22T04:15:26Z

Thereby fixing some rare bugs in tests, maybe in the real world too.

https://issues.apache.org/jira/browse/SOLR-17118

Thereby fixing some rare bugs in tests, maybe in the real world too.

dsmiley · 2024-05-22T04:17:46Z

solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java

  CoreContainer getCoreContainer() throws UnavailableException {
-    waitForCoreContainer(() -> cores, init);


Don't wait at this point anymore; it ought to be ready (so we check to report a nice exception if it isn't)

dsmiley · 2024-05-22T04:19:22Z

solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java

-    return services.computeIfAbsent(key, ServiceHolder::new);
+
+  /** Acquires an instance from the context, waiting if necessary. */
+  public static CoreContainerProvider serviceForContext(ServletContext ctx)


We wait for it; previously did not. This allows us to return a bona-fide CoreContainerProvider instead of a ServiceHolder wrapper. Alas, even CCP is itself a wrapper to what we really want.

solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java

dsmiley · 2024-05-22T04:22:05Z

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

            @Override
            public synchronized void lifeCycleStarted(LifeCycle arg0) {
+              // awkwardly, parts of Solr want to know the port but we don't know that until now


probably something we could fix. The Jetty server inserts itself into the context, thus CoreContainerProvider should be able to pull it out on initialization in order to find the port.

solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java

dsmiley · 2024-05-22T04:23:37Z

solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java

-  // TODO: see if we can get rid of the holder here (Servlet spec actually guarantees
-  // ContextListeners run before filter init, but JettySolrRunner that we use for tests is
-  // complicated)
-  private ServiceHolder coreService;


good riddance

This was the sticky bit. Before pushing I suggest you beast the tests for a good while. My memory is that without this I got a low level of failures and or deadlocks. The current state while apparently not perfect had a low enough frequency that it didn't crop up. (though I don't remember exactly how long I was beasting things either).

Can you please recommend the beasting options I should use today/tonight?

I'd like to merge this to main only and then only backport after awhile.

Fix checkReady.

gus-asf · 2024-05-22T13:14:56Z

solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java

+  public static CoreContainerProvider serviceForContext(ServletContext ctx)
+      throws InterruptedException {
+    long startWait = System.nanoTime();
+    synchronized (ctx) {


Hmm interesting idea, though slightly scary to wait on something jetty owns. Never know when they might also (try to) lock it during server initialization. Also I'm unsure if this would be safe if we ever had another ServletContextListner do the same thing... Worse yet the use of JettySolrRunner for our tests means that we don't have any tests for what start.jar is doing. That dichotomy is a lurking problem, for which there are 2 possible solutions: 1) make jetty startup quicker for tests with https://eclipse.dev/jetty/documentation/jetty-12/operations-guide/index.html#og-quickstart and ditch JettySolrRunner or 2) ditching start.jar and transitioning to custom programatic jetty startup (possibly a variant of JettySolrRunner). As it stands, the tests don't do a good job of covering the true startup process.

@epugh was the one who mentioned the jetty quickstart mechanism to me back when I was doing this. I don't know if he's had thoughts since then.

Worse yet the use of JettySolrRunner for our tests means that we don't have any tests for what start.jar is doing.

BATS integration tests is our answer to that. JettySolrRunner just needs to handle the sequencing realistically (what starts and stops when). You might have also noticed some bolted-on requirements for tests like debug filter, proxy port, and more, so I don't see JettySolrRunner going away. It could be simplified further!

I'm not optimistic about that QuickStart thing helping us -- we've already tuned Jetty. Most notably, our web.xml has metadata-complete="true" thus there is no annotation scanning.

Never know when they might also (try to) lock it during server initialization.

I'm not concerned; it's only Jetty and us to worry about. The code here is resilient to wait being notified inexplicably. There's a loop and it keeps checking for the CoreContainer to show up. Perhaps it'll log a bit more that it's waiting; it's okay. In practice I don't think it'll wait at all since you had pointed out in comments that the ServletContextListener is initialized first.

Based on the initialization sequence that I'm coming to learn, I suspected that this code doesn't even have to wait and I was right! I added an assert before ctx.wait and it never tripped. Ha!

Maybe this is controversial but I'm tempted to remove this code (and the "notify" code elsewhere) and replace it with a lookup that on non-existence throws an IllegalStateException claiming that the ServletContextListener should have done its job by then. If it didn't, something is seriously wrong.

Makes sense to me. If it doesn't work we can always change back right?

… unnatural.

The logic for "we want to shutdown outside of jetty cutting us off" is clearly bogus in its present form.

dsmiley · 2024-05-22T23:30:02Z

solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java

-      assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS));
-
-      // shutdown the core container of new node
-      newJetty.getCoreContainer().shutdown();


I considered this portion of the test invalid since it quite intentionally (see the opening comment) shuts down the container without touching Jetty. That's simply not how Jetty shuts down; only Jetty's lifecycle initiates an orderly shutdown. I could test trying an attempt to do a request against a non-existent server but that seemed silly.

solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

dsmiley

As I said in JIRA, after some more testing I'd like to merge to main then monitor builds carefully. It's better than the current state.

Also I need to run the Nightly tests and Integration tests -- neither run in PR validation.

@markrmiller you might have insights

dsmiley · 2024-05-23T13:12:50Z

solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java

+  public static CoreContainerProvider serviceForContext(ServletContext ctx)
+      throws InterruptedException {
+    long startWait = System.nanoTime();
+    synchronized (ctx) {


Based on the initialization sequence that I'm coming to learn, I suspected that this code doesn't even have to wait and I was right! I added an assert before ctx.wait and it never tripped. Ha!

Maybe this is controversial but I'm tempted to remove this code (and the "notify" code elsewhere) and replace it with a lookup that on non-existence throws an IllegalStateException claiming that the ServletContextListener should have done its job by then. If it didn't, something is seriously wrong.

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

dsmiley · 2024-05-23T13:18:31Z

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

            }
          });
+
+      debugFilter = root.addFilter(DebugFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));


No longer is this section in a LifeCycle.Listener. No longer does it do JettySolrRunner.this.notify(); waitOnSolr is gone.

dsmiley · 2024-05-23T13:19:07Z

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

      QueuedThreadPool qtp = (QueuedThreadPool) server.getThreadPool();
      ReservedThreadExecutor rte = qtp.getBean(ReservedThreadExecutor.class);

      server.stop();

-      if (server.getState().equals(Server.FAILED)) {


because we install these components before Jetty starts in a normal way now

dsmiley · 2024-05-23T13:20:01Z

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

@@ -556,15 +528,7 @@ public synchronized void start(boolean reusePort) throws Exception {
          server.start();
        }
      }
-      synchronized (JettySolrRunner.this) {


after server.start returns, dispatchFilter should be running. It's not started asynchronously; no race.

dsmiley · 2024-05-23T13:22:26Z

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

@@ -699,12 +653,6 @@ public synchronized void stop() throws Exception {
        timeout.waitFor("Timeout waiting for reserved executor to stop.", rte::isStopped);
      }

-      // we want to shutdown outside of jetty cutting us off


as I said in the commit message for this change, this block here has been pointless ever since the previous big change by Gus. The comment dates back to @markrmiller but it's not clear to me way way back then what the concern was. Any way, I've found Jetty to do stuff fairly predictably and in one thread that I can reason about the sequencing.

dsmiley · 2024-05-23T13:22:47Z

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

@@ -726,10 +674,6 @@ public synchronized void stop() throws Exception {
    }
  }

-  private ExecutorService getJettyShutDownThreadPool() {


an aside, I hate supposed getters that in fact yield new things always

dsmiley · 2024-05-28T14:53:46Z

If I get no more feedback, I'll merge Friday.
I'm not sure what single test (or small number of such) to beast. Running all tests isn't viable.

Suggested CHANGES.txt in "Bugs":

Simplify CoreContainerProvider initialization and JettySolrRunner. Avoid deadlock/hang.

dsmiley · 2024-05-31T05:15:31Z

And another hang in another of my PRs: #2487 (comment)

Looking forward to merging this today/Friday :-) and not waiting that long to bring to 9x.

# Conflicts: # solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java

Update dependency commons-cli:commons-cli to v1.8.0 (apache#2481) * SOLR-17137 - Add configuration for SSL between Solr and Prometheus exporter. (apache#2232) When running Solr with mTLS the exporter also needs to use mTLS to be able to talk to Solr api. This commit also adds a bats test that verifies that solr exporter actually scrape the running solr instance by verifying information about a newly made core is present. * Move setShardAttributesToParams (apache#2486) Too special-purpose to warrant any back-compat concern. Was here since 2023-06 note: the test is dubious and admittedly isn't in a great location. * SOLR-16093: Tests: don't require IPv6 (apache#2484) The Java VM/host, doesn't always support IPv6, our tests shouldn't require it. security.policy: Removed 3 lines that were only for tests. * SOLR-17271: PerReplicaState: Shard leader elections still impact state.json (apache#2443) * SOLR-17118: Simplify/fix CoreContainerProvider initialization (apache#2474) Thereby fixing a rare occurrence of Solr hanging on startup. CoreContainerProvider: Don't need any CountDownLatch (x2), the synchronized WeakHashMap of "services", the ServiceHolder, the ContextInitializationKey. No looping to wait for initialization. JettySolrRunner: incorporate the CoreContainerProvider and various servlet filters in a normal way -- add all this before starting, not after. Thus Jetty will shut them down properly so we don't have to. Removed some needless synchronized wait/notify and other needless stuff. HealthCheckHandlerTest was shutting down CoreContainer improperly; this subset of the test was removed. * SOLR-17272: PerReplicaState: Replica "state" and "leader" are still in state.json (apache#2458) * Build: report test history URLs. Fix classMethod suffix (apache#2487) * Test failures reported at the end of a build now include links to view test history at ge.apache.org and fucit.org * Use "./gradlew" (dot-slash) so we can paste the repro line to a shell * SOLR-15447: classMethod suffix is removed Refactored the gradle script some too. * Refactor: dedupe Utils.RANDOM from other spots (apache#2495) * SOLR-17256: Deprecate SolrRequest get/set BasePath and RequestWriter.getPath (apache#2494) Just deprecation. Later, will follow-up with replacements and removal, including CHANGES.txt. * SOLR-17099: snitch does not return spurious tags (apache#2278) * Update dependency org.semver4j:semver4j to v5.3.0 (apache#2440) * Update dependency com.fasterxml.jackson:jackson-bom to v2.17.1 (apache#2346) * Update dependency io.swagger.core.v3:swagger-annotations-jakarta to v2.2.22 (apache#2373) * Update org.mockito:mockito* to v5.12.0 (apache#2490) * Add 'score' field caveat to cursormark docs (apache#2492) Cursormark can give some funky results if used across multiple replicas in a SolrCloud collection, if `score` is used as a sort field. This commit documents this limitation in the cursormark docs. * SOLR-17304: PKG_VERSIONS not honored when loading the schema plugins (apache#2471) Add a reproducer test and a fix. * SOLR-17313: Remove deprecated class SolrLogPostTool (apache#2485) * Remove deprecated class * SOLR-17317: Correct 'wt' interpretation for v2 APIs (apache#2496) Prior to this commit a bug was causing 'wt=json' to return XML content. This is now fixed - the problem was a missing clause in a switch-case statement. * sorl #5: Migration of Thread page into a Thread Component. * sorl #5: Migration of Thread page into a Thread Component. --------- Co-authored-by: Solr Bot <125606113+solrbot@users.noreply.github.com> Co-authored-by: Eivind Bergstøl <eivind@bergstol.no> Co-authored-by: David Smiley <dsmiley@apache.org> Co-authored-by: David Smiley <dsmiley@salesforce.com> Co-authored-by: Noble Paul <noblepaul@users.noreply.github.com> Co-authored-by: Pierre Salagnac <82967811+psalagnac@users.noreply.github.com> Co-authored-by: Jason Gerlowski <gerlowskija@apache.org> Co-authored-by: Andrey Bozhko <andybozhko@gmail.com> Co-authored-by: Eric Pugh <epugh@opensourceconnections.com> Co-authored-by: Nicholas J <nick87@byui.edu>

Thereby fixing a rare occurrence of Solr hanging on startup. CoreContainerProvider: Don't need any CountDownLatch (x2), the synchronized WeakHashMap of "services", the ServiceHolder, the ContextInitializationKey. No looping to wait for initialization. JettySolrRunner: incorporate the CoreContainerProvider and various servlet filters in a normal way -- add all this before starting, not after. Thus Jetty will shut them down properly so we don't have to. Removed some needless synchronized wait/notify and other needless stuff. HealthCheckHandlerTest was shutting down CoreContainer improperly; this subset of the test was removed. (cherry picked from commit 1177796)

SOLR-17118: Simplify CoreContainerProvider initialization

fb1e0c2

Thereby fixing some rare bugs in tests, maybe in the real world too.

dsmiley requested a review from gus-asf May 22, 2024 04:15

github-actions bot added test-framework jetty-server labels May 22, 2024

dsmiley commented May 22, 2024

View reviewed changes

Fix test.

fda5b7c

Fix checkReady.

github-actions bot added tests cat:cloud labels May 22, 2024

gus-asf reviewed May 22, 2024

View reviewed changes

dsmiley added 3 commits May 22, 2024 18:57

Nobody should reach underneath Jetty to shut down CoreContainer; it's…

8d86102

… unnatural.

JettySolrRunner: register CCP normally.

dfac2c0

The logic for "we want to shutdown outside of jetty cutting us off" is clearly bogus in its present form.

getCores() doesn't return null

27fec10

dsmiley commented May 22, 2024

View reviewed changes

JettySolrRunner: simplify lifecycle

e82d6f8

dsmiley requested a review from markrmiller May 23, 2024 01:42

dsmiley commented May 23, 2024

View reviewed changes

More simplification; no wait & notify!

e76c1d1

dsmiley mentioned this pull request May 24, 2024

SOLR-15438: Simplify SolrDispatchFilter.destroy #155

Closed

dsmiley mentioned this pull request May 31, 2024

Build: report test history via GE #2487

Merged

dsmiley added 2 commits May 31, 2024 15:48

Merge branch 'main' into SOLR-17118-CoreContainerProvider-simplify

fe397a2

# Conflicts: # solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java

CHANGES.txt

5fc5345

dsmiley merged commit 1177796 into apache:main Jun 1, 2024
3 checks passed

dsmiley deleted the SOLR-17118-CoreContainerProvider-simplify branch June 1, 2024 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17118: Simplify CoreContainerProvider initialization #2474

SOLR-17118: Simplify CoreContainerProvider initialization #2474

dsmiley commented May 22, 2024

dsmiley May 22, 2024

dsmiley May 22, 2024

dsmiley May 22, 2024

dsmiley May 22, 2024

gus-asf May 22, 2024

dsmiley May 22, 2024

gus-asf May 22, 2024 •

edited

Loading

gus-asf May 22, 2024

dsmiley May 22, 2024

dsmiley May 22, 2024

dsmiley May 23, 2024

epugh May 23, 2024

dsmiley May 22, 2024

dsmiley left a comment

dsmiley May 23, 2024

dsmiley May 23, 2024

dsmiley May 23, 2024

dsmiley May 23, 2024

dsmiley May 23, 2024

dsmiley May 23, 2024

dsmiley commented May 28, 2024

dsmiley commented May 31, 2024

		CoreContainer getCoreContainer() throws UnavailableException {
		waitForCoreContainer(() -> cores, init);

SOLR-17118: Simplify CoreContainerProvider initialization #2474

SOLR-17118: Simplify CoreContainerProvider initialization #2474

Conversation

dsmiley commented May 22, 2024

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

gus-asf May 22, 2024 • 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

dsmiley left a 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

dsmiley commented May 28, 2024

dsmiley commented May 31, 2024

gus-asf May 22, 2024 •

edited

Loading