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

Convert rest-java to servlet with virtual threads #7488

Conversation

jnels124
Copy link
Contributor

@jnels124 jnels124 commented Jan 5, 2024

Description:

  • convert to use spring web with virtual threads instead of webflux
  • update logging filter to be compatible with servlet app
  • update tests to use mockmvc for controller testing

Related issue(s):

Fixes #7465

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
@jnels124 jnels124 added enhancement Type: New feature rest-java Area: Java REST API labels Jan 5, 2024
@jnels124 jnels124 added this to the 0.97.0 milestone Jan 5, 2024
@jnels124 jnels124 self-assigned this Jan 5, 2024
@jnels124 jnels124 linked an issue Jan 5, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1b65f4a) 92.44% compared to head (93a5620) 92.47%.
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7488      +/-   ##
============================================
+ Coverage     92.44%   92.47%   +0.02%     
+ Complexity     6883     6876       -7     
============================================
  Files           867      867              
  Lines         28638    28567      -71     
  Branches       3353     3336      -17     
============================================
- Hits          26475    26416      -59     
+ Misses         1392     1382      -10     
+ Partials        771      769       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
@@ -45,23 +34,4 @@ MeterBinder processMemoryMetrics() {
MeterBinder processThreadMetrics() {
return new ProcessThreadMetrics();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tomcat doesn't provide an easy to duplicate this functionality. We would have to implement. Although not sure it is worth it. We already have server_http metrics from spring. Uris are slightly different but for our use case we can derive the same information from the server_http metrics.

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
port: 8084
shutdown: graceful
tomcat:
connection-timeout: 3s
forward-headers-strategy: framework #Enable spring ForwardedHeaderFilter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will wrap enable a filter that wraps the request with a request object that utilizes forwarded for headers for common methods like getRemoteAddr()

@jnels124 jnels124 marked this pull request as ready for review January 6, 2024 02:17
hedera-mirror-rest-java/src/main/resources/application.yml Outdated Show resolved Hide resolved
hedera-mirror-rest-java/build.gradle.kts Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/java-conventions.gradle.kts Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/spring-conventions.gradle.kts Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
@steven-sheehy steven-sheehy changed the title convert rest java to servlet with virtual threads Convert rest-java to servlet with virtual threads Jan 7, 2024
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
.thenAwait(WAIT)
.expectComplete()
.verify(WAIT);
loggingFilter.doFilter(request, response, new MockFilterChain());
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
loggingFilter.doFilter(request, response, new MockFilterChain());
loggingFilter.doFilter(request, response, chain);

void filterOnCancel(CapturedOutput output) {
var exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/").build());
var request = new MockHttpServletRequest("GET", "/");
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
;

hedera-mirror-rest-java/build.gradle.kts Outdated Show resolved Hide resolved
// Disable serial and this-escape warnings due to errors in generated code
options.compilerArgs.addAll(listOf("-Werror", "-Xlint:all", "-Xlint:-serial,-this-escape"))
// Disable deprecation, serial, and this-escape warnings due to errors in generated code
options.compilerArgs.addAll(listOf("-Werror", "-Xlint:all", "-Xlint:-deprecation,-serial,-this-escape"))
Copy link
Member

Choose a reason for hiding this comment

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

I assume it's again the openapi gradle plugin using deprecated methods? We should file a bug upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes will do.. Getting a minimal example put together and will file a bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Copy link

sonarcloud bot commented Jan 8, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@xin-hedera xin-hedera left a comment

Choose a reason for hiding this comment

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

LGTM

@jnels124 jnels124 merged commit ca22174 into main Jan 8, 2024
27 checks passed
@jnels124 jnels124 deleted the 7465-change-rest-java-module-to-use-spring-mvc-and-virtual-threads branch January 8, 2024 19:23
bilyana-gospodinova pushed a commit that referenced this pull request Jan 19, 2024
* convert to use spring web with virtual threads instead of webflux
* update logging filter to be compatible with servlet app
* update tests to use mockmvc for controller testing
* enable spring ForwardedHeaderFilter filter

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
bilyana-gospodinova pushed a commit that referenced this pull request Jan 24, 2024
* convert to use spring web with virtual threads instead of webflux
* update logging filter to be compatible with servlet app
* update tests to use mockmvc for controller testing
* enable spring ForwardedHeaderFilter filter

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type: New feature rest-java Area: Java REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change rest-java module to use spring-mvc and virtual threads
3 participants