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 #12047 allow disabling opening connectors before starting #12049

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

kelunik
Copy link
Contributor

@kelunik kelunik commented Jul 16, 2024

See #12047.

@joakime
Copy link
Contributor

joakime commented Jul 16, 2024

Looking good.
Can you get a ECA lined up so we can accept this PR?

See https://github.com/jetty/jetty.project/blob/jetty-12.0.x/CONTRIBUTING.md#eclipse-contributor-agreement

@kelunik
Copy link
Contributor Author

kelunik commented Jul 24, 2024

Yes, I'll try to get the ECA sorted.

@joakime joakime self-assigned this Jul 25, 2024
@joakime
Copy link
Contributor

joakime commented Jul 25, 2024

@kelunik if you can get your ECA sorted today, then this could show up in the next release (12.0.12)
Otherwise it will have to wait till 12.0.13 (or even 12.1.0)

@joakime joakime closed this Jul 25, 2024
@joakime joakime reopened this Jul 25, 2024
@joakime
Copy link
Contributor

joakime commented Jul 25, 2024

Oops, wrong button. Reopened!

@kelunik
Copy link
Contributor Author

kelunik commented Jul 25, 2024

@joakime Unless I redeclare this as a personal contribution, I can't make this happen today. But I'm also not in a hurry as the workaround works fine for me, so shipping in a later release is OK.

Anything I can do for the failing jenkins steps?

@kelunik
Copy link
Contributor Author

kelunik commented Aug 23, 2024

@joakime The ECA is now signed!

@joakime joakime requested a review from gregw August 26, 2024 18:31
@joakime
Copy link
Contributor

joakime commented Aug 26, 2024

@gregw you ok with this change?
The QoSHandlerTest failures here are already fixed in HEAD.

@gregw
Copy link
Contributor

gregw commented Aug 26, 2024

The QoSHandlerTest failures here are already fixed in HEAD.

Actually I just disabled the tests for now....

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'm a bit dismayed that webapplications can take soooo long to start that this is an issue.

Perhaps trying the quickstart mechanism to prescan all your webapps would be a better approach? It should only take a few 100 micro seconds to start such a server.

But if this is really needed, I have a quibble about the name.

@joakime
Copy link
Contributor

joakime commented Aug 27, 2024

Perhaps trying the quickstart mechanism to prescan all your webapps would be a better approach? It should only take a few 100 micro seconds to start such a server.

For a webapp that depends only on Servlet behaviors, yes, this is very possible.

But if you have a webapp that does scanning from multiple 3rd party libraries (eg: certain spring configurations, specific hibernate configurations, some jersey configs, etc) then each of those adds to the time to startup because each of those will scan the entire webapp bytecode for things like interfaces, annotations, etc.

Even though the modern era of webapp development can be quite complicated, don't worry too much about the details.

Just try the Jetty QuickStart mechanism (for your ee# environment) and see how it helps.

https://jetty.org/docs/jetty/12/operations-guide/quickstart/index.html

@joakime
Copy link
Contributor

joakime commented Aug 27, 2024

Know that you can also tweak what jars are actually scanned on the Container classloader and the Webapp classloader with configuration.

This is an alternative to using quickstart.

See https://jetty.org/docs/jetty/12/operations-guide/annotations/index.html#scanning

@gregw
Copy link
Contributor

gregw commented Aug 28, 2024

@kelunik we've missed the deadline for the current release cycle. Moving this to next months release.
If you can update the names, then I'm inclined to merge it in that cycle.

@kelunik
Copy link
Contributor Author

kelunik commented Sep 2, 2024

I'm a bit dismayed that webapplications can take soooo long to start that this is an issue.

Perhaps trying the quickstart mechanism to prescan all your webapps would be a better approach? It should only take a few 100 micro seconds to start such a server.

Scanning isn't what's causing our server to need time to start. It's the Spring context that loads lots of data into memory on start. But even a few seconds to start would cause additional latency in our load balancer setup, because once the socket is open, the load balancer tries to send requests (due to passive healthchecks).

I've adjusted the naming and rebased onto the latest 12.x branch, so hopefully everything is fine now.

@gregw gregw merged commit 1d9f108 into jetty:jetty-12.0.x Sep 24, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants