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 - Refactor Local Deployment Scanning #12583

Open
wants to merge 173 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.
Moved optional Environment XML resolution from ${jetty.base}/webapps/<env>.xml to ${jetty.base}/environments/<env>.xml to avoid name collision with deployable XML contexts.

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

@gregw
Copy link
Contributor

gregw commented Mar 12, 2025

@joakime @sbordet I've almost convinced myself that we should do the following renames:

  • DirectDeployer -> StandardDeployer
  • PathContextHandlerFactory -> StandardContextHandlerFactory

I could be convinced to do s/Standard/Default/. Yes I know I've argued against both of these before.... but if both the interface implementations are named the same way, it makes more sense.

@joakime
Copy link
Contributor Author

joakime commented Mar 12, 2025

@joakime @sbordet I've almost convinced myself that we should do the following renames:

* `DirectDeployer` -> `StandardDeployer`

* `PathContextHandlerFactory` -> `StandardContextHandlerFactory`

I could be convinced to do s/Standard/Default/. Yes I know I've argued against both of these before.... but if both the interface implementations are named the same way, it makes more sense.

Looking at the current jetty-deploy, we see only the following java files now ...

[joakim@hyperion 12.1.x][fix/12.1.x/unify-deploy*]$ ls -la jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/
total 116
drwxrwxr-x 2 joakim joakim  4096 Mar 12 08:59 .
drwxrwxr-x 3 joakim joakim  4096 Mar  3  2022 ..
-rw-rw-r-- 1 joakim joakim  3662 Mar 12 08:59 ContextHandlerFactory.java
-rw-rw-r-- 1 joakim joakim  5938 Mar 12 08:39 Deployer.java
-rw-rw-r-- 1 joakim joakim 61407 Mar 12 08:42 DeploymentScanner.java
-rw-rw-r-- 1 joakim joakim 10159 Mar 12 08:39 DirectDeployer.java
-rw-rw-r-- 1 joakim joakim   647 Mar 12 08:41 package-info.java
-rw-rw-r-- 1 joakim joakim 18949 Mar 10 11:18 PathsContextHandlerFactory.java

I can see it just being 4 files ...

  • Deployer - what the DirectDeployer is now
  • ContextScanner - what the DeploymentScanner is
  • StandardContextHandlerFactory - what the PathsContextHandlerFactory is
  • ContextHandlerFactory - no change

@janbartel
Copy link
Contributor

I can see it just being 4 files ...

  • Deployer - what the DirectDeployer is now
  • ContextScanner - what the DeploymentScanner is
  • StandardContextHandlerFactory - what the PathsContextHandlerFactory is
  • ContextHandlerFactory - no change

It still seems the wrong way around that we configure something called a Scanner that then hands off to a Deployer. It still seems much more correct to me to configure something called Deployer that has-a Scanner.

@joakime
Copy link
Contributor Author

joakime commented Mar 12, 2025

It still seems the wrong way around that we configure something called a Scanner that then hands off to a Deployer. It still seems much more correct to me to configure something called Deployer that has-a Scanner.

It's a separation of concerns.
Talking about this in FileSystem terms ...

  • Scanner is the source
  • Deployer is the action - Files.copy(src,dest)
  • ContextHandlerCollection is the destination.

@gregw
Copy link
Contributor

gregw commented Mar 12, 2025

I can see it just being 4 files ...

  • Deployer - what the DirectDeployer is now

That only works if we lose the interface. We can do that, but then we should do the same with the ContextHandlerFactory interface.
The interface is a good place to define the listeners, but they could also be done at the top level

  • ContextScanner - what the DeploymentScanner is
    I think DeploymentScanner is about the only name I'm 100% happy with. Leave it!
  • StandardContextHandlerFactory - what the PathsContextHandlerFactory is
  • ContextHandlerFactory - no change
    Again - whatever pattern we use here, we should use the same pattern for the Deployer.

So I see

Option 1

  • interface Deployer
    • interface Listener
  • interface ContextHandlerCollection
  • class DeploymentScanner
  • class StandardDeployer implements Deployer
  • class StandardContextHandlerFactory implements ContextHandlerFactory

Option 2

  • class DeploymentScanner
  • class Deployer
    • interface Listener
  • class ContextHandlerFactory

Option 2 is simplest. A user can still extend Deployer and CHF, but they get the standard implementations as their super, because there is no interface.
So I'm 40% Option 1; 60% option 2

@janbartel @sbordet @joakime thoughts?

@sbordet
Copy link
Contributor

sbordet commented Mar 12, 2025

@gregw I'm 90% option 1.

With option 1, IIUC, OSGi will be an OSGiDeployerListener + OSGiContextHandlerFactory.

With option 2, OSGiContextHandlerFactory extends StandardContextHandlerFactory, again IIUC it will carry over a bunch of "standard" things that are not necessary for OSGi, am I right? If so, I'd prefer it to be an interface, although it is very specific to file system.

Also, I like that Deployer is an interface, as it can be implemented as a REST call for fancy deployments: you can have a local file scanner that monitors files that, when they change, get uploaded somewhere for the real deployment.

@gregw
Copy link
Contributor

gregw commented Mar 12, 2025

@gregw I'm 90% option 1.

OK so now I'm 70% option 1 30% option 2. Any other bidders?

@gregw
Copy link
Contributor

gregw commented Mar 13, 2025

@gregw I'm 90% option 1.

OK so now I'm 70% option 1 30% option 2. Any other bidders?

I did the rename to option 1 and did a first pass through the documentation.

Comment on lines +43 to +57
/**
* Attributes that start with this name return a String pointing to XML on the filesystem that should be used for Environment based
* modifications to the ContextHandler that was created.
*
* <pre>
* jetty.deploy.environmentXml=config/base-ee8.xml
* jetty.deploy.environmentXml.auditing=config/auditing-ee8.xml
* </pre>
*/
String ENVIRONMENT_XML_ATTRIBUTE = "jetty.deploy.environmentXml";
/**
* Attribute name that stores the {@code List<Path>} pointing to XML files that represent
* the Environment XML files to apply to the ContextHandler has been created.
*/
String ENVIRONMENT_XML_PATHS_ATTRIBUTE = "jetty.deploy.paths.environmentXmls";
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have the environments directory, is this mechanism still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first one can go.
The second one is the result of using the /environments/ directory, so I would like to keep that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the first one can be used to have context specific environment XML.
Not sure the value of that nuanced behavior is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second one is almost an internal implementation detail, not to be used by users.


//ensure a deployer
StandardDeployer deployer = ensureDeployer(server);
deployer.addEventListener(new OSGiDeploymentListener());
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a risk that the deployer might already exist and already have an OSGIDeploymentListener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, have to noodle out a better way for you then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2194b55

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I think this is good enough to commit. We can refine we other PRs.

@gregw gregw requested review from janbartel and sbordet March 14, 2025 07:09
@joakime
Copy link
Contributor Author

joakime commented Mar 14, 2025

@gregw So, for the other branch where we fix the git history here (like file renames/moves), how about this ...

Renames in jetty-core/jetty-deploy

From Jetty 12.0.x To Jetty 12.1.x
DeploymentManager.java StandardDeployer.java
providers/ScanningAppProvider.java DeploymentScanner.java
providers/ContextProvider.java StandardContextHandlerFactory.java

I don't think there's really going to be much else to focus on.
Thoughts?

joakime added a commit that referenced this pull request Mar 14, 2025
@joakime
Copy link
Contributor Author

joakime commented Mar 14, 2025

I've created a new branch called fix/12.1.x/deployer-refactor-rename-fixes that has the changes from this branch including proper renames/moves of the relevant files.

If you pull from origin, you can verify that these branches are the same, content wise, via the command line ...

git diff origin/fix/12.1.x/deployer-refactor-rename-fixes...origin/fix/12.1.x/unify-deploy

@gregw
Copy link
Contributor

gregw commented Mar 16, 2025

I've created a new branch called fix/12.1.x/deployer-refactor-rename-fixes that has the changes from this branch including proper renames/moves of the relevant files.

If you pull from origin, you can verify that these branches are the same, content wise, via the command line ...

git diff origin/fix/12.1.x/deployer-refactor-rename-fixes...origin/fix/12.1.x/unify-deploy

Maybe make a new PR from https://github.com/jetty/jetty.project/tree/fix/12.1.x/deployer-refactor-rename-fixes

@joakime
Copy link
Contributor Author

joakime commented Mar 17, 2025

I've created a new branch called fix/12.1.x/deployer-refactor-rename-fixes that has the changes from this branch including proper renames/moves of the relevant files.
If you pull from origin, you can verify that these branches are the same, content wise, via the command line ...

git diff origin/fix/12.1.x/deployer-refactor-rename-fixes...origin/fix/12.1.x/unify-deploy

Maybe make a new PR from https://github.com/jetty/jetty.project/tree/fix/12.1.x/deployer-refactor-rename-fixes

Done.

I had to create a new branch, as I was unhappy with the result of the older one.
New Branch: fix/12.1.x/deploy-refactor-minimal-commits

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 review
Development

Successfully merging this pull request may close these issues.

Review DeploymentManager and ScanningAppProvider
6 participants