Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #10437 - Unify Deployer ContextProvider #12583

Open
wants to merge 26 commits into
base: jetty-12.1.x
Choose a base branch
from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Nov 26, 2024

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)

@joakime joakime added Enhancement Bug For general bugs on Jetty side labels Nov 26, 2024
@joakime joakime requested a review from sbordet November 26, 2024 21:48
@joakime joakime self-assigned this Nov 26, 2024
@joakime joakime changed the base branch from jetty-12.0.x to jetty-12.1.x November 26, 2024 21:48
@joakime joakime linked an issue Nov 26, 2024 that may be closed by this pull request
@joakime joakime marked this pull request as ready for review December 3, 2024 00:01
@janbartel janbartel requested a review from gregw December 10, 2024 04:35
@@ -641,4 +580,308 @@ protected void pathChanged(Path path) throws Exception
if (isDeployable(path))
super.pathChanged(path);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<Ref refid="DeploymentManager">
<Call name="addAppProvider">
<Arg>
<Ref refid="contextProvider" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +70 to +71
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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +83 to +84
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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +64 to +65
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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sbordet
Copy link
Contributor

sbordet commented Dec 10, 2024

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

sbordet commented Dec 10, 2024

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
Copy link
Contributor

gregw commented Dec 17, 2024

@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
Copy link
Contributor

gregw commented Dec 17, 2024

@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:

  1. 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).
  2. 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.)

@joakime
Copy link
Contributor Author

joakime commented Dec 18, 2024

@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:

  1. 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).

  2. 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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

firoj0

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Enhancement
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

Review DeploymentManager and ScanningAppProvider
5 participants