Issue #10437 - Unify Deployer ContextProvider #12583

joakime · 2024-11-26T21:48:34Z

A single scanner for all Environments.
Environment attributes are how the environment specific deployment configuration is controlled.
Existing properties behaviors maintained.

Currently a WIP (needs more testing and documentation)

…ify-deploy

...-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ScanningAppProvider.java

janbartel · 2024-12-10T05:21:08Z

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

@@ -641,4  580,308 @@ protected void pathChanged(Path path) throws Exception
        if (isDeployable(path))
            super.pathChanged(path);
    }
+


For the isDeployable method, as you're doing a lot of rewriting, it may be worth looking into #12543. The problem here is that on a hot redeploy via touching just the war (rather than the associated xml file), line 562 will cause the redeploy not to happen. That line works fine on initial deployment, but not on a redeploy.

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

janbartel · 2024-12-10T05:41:43Z

jetty-core/jetty-deploy/src/main/config/etc/jetty-deploy.xml

+  <Ref refid="DeploymentManager">
+    <Call name="addAppProvider">
+      <Arg>
+        <Ref refid="contextProvider" />


Should it be an error to configure more than one ContextProvider that is scanning the same directory?

I wouldn't consider that an error.
Sure, it is a weird configuration.
Plus, I don't want to implement directory scanning lock files to prevent that kind of configuration.

sbordet · 2024-12-10T10:13:11Z

...loy/src/test/java/org/eclipse/jetty/deploy/providers/ContextProviderDeferredStartupTest.java

+        jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deployment-manager.xml"));
+        jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deploy.xml"));


Why these need to be referenced by the source, and the others don't?

These should be there for the test to function.
But instead of doing the 12.0.x technique of duplicating these into src/test/resources and having to keep them in sync, it just made more sense to reference the actual XML files directly instead.

sbordet · 2024-12-10T10:13:31Z

...ploy/src/test/java/org/eclipse/jetty/deploy/providers/ContextProviderRuntimeUpdatesTest.java

+        jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deployment-manager.xml"));
+        jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deploy.xml"));


These should be there for the test to function.
But instead of doing the 12.0.x technique of duplicating these into src/test/resources and having to keep them in sync, it just made more sense to reference the actual XML files directly instead.

sbordet · 2024-12-10T10:13:38Z

...etty-deploy/src/test/java/org/eclipse/jetty/deploy/providers/ContextProviderStartupTest.java

+        jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deployment-manager.xml"));
+        jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deploy.xml"));


These should be there for the test to function.
But instead of doing the 12.0.x technique of duplicating these into src/test/resources and having to keep them in sync, it just made more sense to reference the actual XML files directly instead.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Deployable.java

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java

sbordet · 2024-12-10T10:45:21Z

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

-                            tmp.stringPropertyNames().forEach(k -> properties.put(k, tmp.getProperty(k)));
-                        }
-                    }
+                    context = contextClass.getDeclaredConstructor().newInstance();


Constructor should be public, so use:

Suggested change

context = contextClass.getDeclaredConstructor().newInstance();

context = contextClass.getConstructor().newInstance();

I'd also simplify this if/else by removing the else:

if (contextClass == null) throw ...; context = contextClass.getConstructor().newInstance();

sbordet · 2024-12-10T10:51:45Z

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

+            // check if there is a specific ContextHandler type to create set in the
+            // properties associated with the webapp. If there is, we create it _before_
+            // applying the environment xml file.
+            String contextHandlerClassName = (String)appAttributes.getAttribute(Deployable.CONTEXT_HANDLER_CLASS);


It is strange that the ContextHandler is created here, but also down below with the default class.

However, for the default case, the webapp properties are not associated -- does not feel right.

Feels more it should have been:

String contextHandlerClassName = (String)appAttributes.getAttribute(Deployable.CONTEXT_HANDLER_CLASS); if (contextHandlerClassName == null) contextHandlerClassName = (String)appAttributes.getAttribute(Deployable.CONTEXT_HANDLER_CLASS_DEFAULT); ...

Let me try to explain why this code is the way it is on jetty-12.0.x first ...

There is an order of events that we have to deal with.

An App can have a custom ContextHandlerClass.
The XML is then applied to this instance.

If there is no app specific custom ContextHandlerClass, then the XML declared one is used.

The other custom context load below is for when there is no XML deployable, which uses the one from the environment.

Now, with this PR, I think this distinction is no longer relevant.

As the call earlier to Attributes appAttributes = initAttributes(environment, app); will already handle the app specified custom defined property overriding the environment defined property.

By the time the first Load class/new Instance occurs, the app vs environment property decision is already made.
The only thing relevant is that the instance is created before the XMLs are applied (if there are XMLs).

I'm not sure the second Load class / new instance is relevant any longer.
I'll dig into this.

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

jetty-core/jetty-deploy/src/main/config/etc/jetty-deployment-manager.xml

jetty-core/jetty-deploy/src/main/config/modules/deployment-manager.mod

sbordet · 2024-12-10T12:06:13Z

I'd like to see:

alphabetical sort of scanned files
DeploymentManager does not need to know AppProviders. It's AppProviders that call the DeploymentManager to deploy Apps. AppProviders can just be added as beans to DeploymentManager
Documentation
ContextProvider renamed to something more telling. DeploymentScanningAppProvider? The word "Context" has no meaning in this class, and it is lost in the current class name that it is a ScanningAppProvider.

sbordet · 2024-12-10T12:12:43Z

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

+     * @param name the name of the environment.
+     * @return the deployment configuration for the {@link Environment}.
+     */
+    public EnvironmentConfig configureEnvironment(String name)


This is strange, as calling this method returns a different object every time, so calling it twice make unclear whether the previous configuration was retained or not.

Since EnvironmentConfig is just a wrapper around Environment properties (to give compile-time checks), it should be stored as a property itself in the Environment.

This method should be renamed getDeploymentConfiguration(String name), and it is not really an EnvironmentConfig, it is the DeploymentConfiguration for that specific environment.

Since all it does is set/get Environment attributes, having multiple objects isn't going to change anything on the behavior.
In the future, we can change this to be the same EnvironmentConfig object if needed, but this level of change is not required for success in this PR.

sbordet · 2024-12-10T12:16:04Z

One thing to point out in this solution is that it relies on the Environment JVM singleton.

This means it won't be possible to have 2 Server instances in the same JVM, each with its own ContextProvider.

Not really a problem of this PR, because the real issue would is the Environment JVM singleton.

gregw · 2024-12-17T20:50:16Z

@sbordet said:

One thing to point out in this solution is that it relies on the Environment JVM singleton.
This means it won't be possible to have 2 Server instances in the same JVM, each with its own ContextProvider.
Not really a problem of this PR, because the real issue would is the Environment JVM singleton.

Can you explain? The Environment class just keeps each known environment and its Classloader. This doesn't prevent multiple servers or context deployers. Or are you saying that different servers might want different sets of environments available? But that is like saying different servers might want different handlers available, but all the handler classes are on the system classpath.

gregw · 2024-12-17T21:03:23Z

@sbordet said:

alphabetical sort of scanned files

@joakime I think this is an important feature to add at this stage as it will probably need some restructuring. Two approaches that I can see:

modify the scanner (or wrap the scanner) so that the DiscreteListener events just happen to come in order. This could be done with a BulkListener that does the sort and then simulates the DiscreteListener events (although it might be hard to recreate the difference between added and modified).
Change the ScanningAppProvider to be a BulkListener. This would allow the sort to be done in the deployer without changing the Scanner at all. It would also allow the provider to look over the bulk list and removed duplicates (remove a war that has an xml in the list as well, perhaps replace a war with its xml if the war changes but the xml does not, etc.)

...-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ScanningAppProvider.java

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java

…ify-deploy

joakime · 2024-12-18T19:50:37Z

@sbordet said:

alphabetical sort of scanned files

@joakime I think this is an important feature to add at this stage as it will probably need some restructuring. Two approaches that I can see:

modify the scanner (or wrap the scanner) so that the DiscreteListener events just happen to come in order. This could be done with a BulkListener that does the sort and then simulates the DiscreteListener events (although it might be hard to recreate the difference between added and modified).

Change the ScanningAppProvider to be a BulkListener. This would allow the sort to be done in the deployer without changing the Scanner at all. It would also allow the provider to look over the bulk list and removed duplicates (remove a war that has an xml in the list as well, perhaps replace a war with its xml if the war changes but the xml does not, etc.)

I implemented this with the BulkListener approach in commit 5f6d504

gregw · 2024-12-21T21:02:53Z

...-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ScanningAppProvider.java

+            List<Path> sortedPaths = paths.stream()
+                .sorted(PathCollators.byName(true))
+                .toList();
+


I was thinking that at this stage it would be best to resolve xml vs war paths, as you can see all the paths at once.
If there is a foo.xml and/or a foo.war in the original list, then the end result should just be foo.xml to be deployed

joakime added 8 commits November 22, 2024 15:01

Initial pass at implementation of EnvironmentContextProvider

8878567

Fixing javadoc errors

3a46e37

Moving things around

c5c07c2

Moving changes back into ContextProvider

e0bdeb3

Introducing ContextProvider.configureEnvironment(String name)

7588d5f

Fixing <env>[-zzz].properties sorting

42015b3

Allow an Environment to be removed (needed for testing with Environment)

f9c90ce

Fixing test cases

83fc5f1

joakime added Enhancement Bug For general bugs on Jetty side labels Nov 26, 2024

joakime requested a review from sbordet November 26, 2024 21:48

joakime self-assigned this Nov 26, 2024

joakime changed the base branch from jetty-12.0.x to jetty-12.1.x November 26, 2024 21:48

joakime linked an issue Nov 26, 2024 that may be closed by this pull request

Review DeploymentManager and ScanningAppProvider #10437

Open

joakime added 10 commits November 27, 2024 07:45

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/12.1.x/un…

a0c9058

…ify-deploy

Fix javadoc

249679f

Fix method names

bef04d9

Fixing Environment contextClass loading

add337a

Fixing test cases

2e9660d

Fixing quickstart XML references to provider

9bfab80

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/12.1.x/un…

2c651ea

…ify-deploy

Fixing quickstart XML

0e534f7

Fixing deploy containerScanJarPattern for ee9/ee8

43eb679

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/12.1.x/un…

c5a00b8

…ify-deploy

joakime marked this pull request as ready for review December 3, 2024 00:01

janbartel requested a review from gregw December 10, 2024 04:35

janbartel requested changes Dec 10, 2024

View reviewed changes

sbordet requested changes Dec 10, 2024

View reviewed changes

gregw requested changes Dec 17, 2024

View reviewed changes

...-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ScanningAppProvider.java Outdated Show resolved Hide resolved

jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java Show resolved Hide resolved

joakime added 3 commits December 18, 2024 10:53

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/12.1.x/un…

229e358

…ify-deploy

Changes from reviews. Preparing for BulkListener impl.

a363189

Using BulkListener with sorted paths for alphabetical deployment.

5f6d504

joakime added 5 commits December 18, 2024 13:54

Changes from review

77c34c2

More changes from review

5306c3d

Simplify else

96d5bec

Reworking deploy.createContextHandler to be easier to follow

ee18845

Fixing WebAppContext default-context-path mistake

e498f2c

gregw requested changes Dec 21, 2024

View reviewed changes

This comment was marked as spam.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #10437 - Unify Deployer ContextProvider #12583

Issue #10437 - Unify Deployer ContextProvider #12583

joakime commented Nov 26, 2024

janbartel Dec 10, 2024

janbartel Dec 10, 2024

joakime Dec 18, 2024

sbordet Dec 10, 2024

joakime Dec 18, 2024

sbordet Dec 10, 2024

joakime Dec 18, 2024

sbordet Dec 10, 2024

joakime Dec 18, 2024

sbordet Dec 10, 2024

sbordet Dec 10, 2024

joakime Dec 18, 2024

sbordet commented Dec 10, 2024 •

edited

Loading

sbordet Dec 10, 2024

joakime Dec 18, 2024

sbordet commented Dec 10, 2024

gregw commented Dec 17, 2024

gregw commented Dec 17, 2024

joakime commented Dec 18, 2024 •

edited

Loading

gregw Dec 21, 2024

This comment was marked as spam.

		jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deployment-manager.xml"));
		jetty.addConfiguration(MavenPaths.projectBase().resolve("src/main/config/etc/jetty-deploy.xml"));

	context = contextClass.getDeclaredConstructor().newInstance();
	context = contextClass.getConstructor().newInstance();

Issue #10437 - Unify Deployer ContextProvider #12583

Are you sure you want to change the base?

Issue #10437 - Unify Deployer ContextProvider #12583

Conversation

joakime commented Nov 26, 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

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

sbordet commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet commented Dec 10, 2024

gregw commented Dec 17, 2024

gregw commented Dec 17, 2024

joakime commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as spam.

sbordet commented Dec 10, 2024 •

edited

Loading

joakime commented Dec 18, 2024 •

edited

Loading