-
Notifications
You must be signed in to change notification settings - Fork 66
chore(implementation): use Jetty-12.1 core without servlets #333
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
base: main
Are you sure you want to change the base?
chore(implementation): use Jetty-12.1 core without servlets #333
Conversation
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
|
Thanks! Also all references with maven.compiler.source>11</maven.compiler.source> need to change to 17 |
|
Maybe add java21 or 25 |
|
@akerekes please help on google internal kokoro workflows. |
|
Hi @lachlan-roberts , thank you for the PR! I'll work on the Google internal side. One note on the timeline is that Cloud Function deprecates java11 at the end of the month: https://cloud.google.com/functions/docs/runtime-support#java so my preference is to have this PR ready to merge, but wait until the end of the month with the merge, then do it in early November and release a new version of the FF-java library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lachlan-roberts for the PR! I did the first round of review.
Please rebase the PR to HEAD one more time, I'm not planning to merge any other PRs until this one is done to avoid further rebasing and merge conflicts.
invoker/core/src/main/java/com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java
Outdated
Show resolved
Hide resolved
invoker/core/src/main/java/com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java
Outdated
Show resolved
Hide resolved
invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpRequestImpl.java
Outdated
Show resolved
Hide resolved
invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpRequestImpl.java
Show resolved
Hide resolved
invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpRequestImpl.java
Outdated
Show resolved
Hide resolved
invoker/core/src/test/java/com/google/cloud/functions/invoker/http/HttpTest.java
Outdated
Show resolved
Hide resolved
invoker/core/src/test/java/com/google/cloud/functions/invoker/http/HttpTest.java
Outdated
Show resolved
Hide resolved
invoker/core/src/test/java/com/google/cloud/functions/invoker/http/HttpTest.java
Outdated
Show resolved
Hide resolved
invoker/core/src/test/java/com/google/cloud/functions/invoker/http/HttpTest.java
Show resolved
Hide resolved
invoker/core/src/test/java/com/google/cloud/functions/invoker/http/HttpTest.java
Outdated
Show resolved
Hide resolved
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
invoker/core/src/main/java/com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java
Show resolved
Hide resolved
invoker/core/src/main/java/com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java
Show resolved
Hide resolved
| new TimerTask() { | ||
| @Override | ||
| public void run() { | ||
| // TODO: there is a race between the handler writing response and timeout firing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the action item to this TODO? what needs to be done to address this race condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The race condition is intrinsic to this approach, and was also present using the original TimeoutFilter. And there were also various other problems with the original TimeoutFilter.
I have pushed another commit to improve this.
- Before the timeout was per call to
doFilter, it was not measuring the total time for the handling of the request/response to complete. - It used to create a new
Timerfor every call todoFilter()(possibly multiple times per request), now it reuses theSchedulerof the server and only once per request.
|
@lachlan-roberts Thank you for the follow-up. Can you reformat the code to make the linter check pass? The PR looks good to me, I'm waiting on @maemayve 's comments to be resolved and happy to approve it then. |
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the timeout behavior change breaks the test, @lachlan-roberts can you take a look?
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
|
@akerekes yep thanks, the Should be passing tests now after latest commit (it passed locally). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's a flakiness in the tests: https://github.com/GoogleCloudPlatform/functions-framework-java/actions/runs/19053493656/job/54419863348?pr=333 which passed on re-run (I'm not sure if the changes in this PR have any effect on it, wdyt @lachlan-roberts ?)
And there are some formatting issues with the code: https://github.com/GoogleCloudPlatform/functions-framework-java/actions/runs/19053493578/job/54419854427?pr=333
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
To me this looks like a race condition in the test. The main thread is sending a request to the server, waiting for the response, then checking the server logs to see if the required output is there. But those server logs are made available in a So just because the response is received doesn't mean the log lines are in the |
This is based on the work from #235.
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.
BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Replaces