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

Support @LocalArmeriaPort, @LocalArmeriaPorts #2649

Closed
wants to merge 11 commits into from

Conversation

heowc
Copy link
Contributor

@heowc heowc commented Apr 4, 2020

Motivation:

  • It provides a convenient annotation for working with test cases.

Modifications:

  • Add @LocalArmeriaPort, @LocalArmeriaPorts
  • Add ArmeriaServerPortInfoApplicationContextInitializer to spring.factories
  • Change webflux's application context
  • Add event publisher to forward events that reset port-related environment properties

Result:

@heowc
Copy link
Contributor Author

heowc commented Apr 4, 2020

I found that the test failed 176e8a1. However, it does not seem to be related to the PR.

java 8

> Task :brave:shadedTest
BraveClientIntegrationTest > clientTimestampAndDurationEnclosedByParent[0] FAILED
    java.lang.AssertionError: Expected {"traceId":"981a550bd1154784","id":"981a550bd1154784","name":"parent","timestamp":1585977733006000,"duration":9938,"localEndpoint":{"serviceName":"unknown","ipv4":"192.168.240.2"}} to finish after {"traceId":"981a550bd1154784","parentId":"981a550bd1154784","id":"59a9f75158d93b30","kind":"CLIENT","name":"get","timestamp":1585977733006000,"duration":10153,"localEndpoint":{"serviceName":"unknown","ipv4":"192.168.240.2"},"remoteEndpoint":{"ipv4":"127.0.0.1","port":42437},"annotations":[{"timestamp":1585977733007000,"value":"connection-acquire.start"},{"timestamp":1585977733007000,"value":"socket-connect.start"},{"timestamp":1585977733012123,"value":"socket-connect.end"},{"timestamp":1585977733012654,"value":"connection-acquire.end"},{"timestamp":1585977733014894,"value":"ws"},{"timestamp":1585977733015638,"value":"wr"}],"tags":{"http.method":"GET","http.path":"/foo"}}

java 11 (run task locally)

Execution failed for task ':it:spring:boot-tomcat8.5:javadoc'.
> Javadoc generation failed. Generated Javadoc options file (useful for troubleshooting): '/Users/heowc/Projects/armeria/it/spring/boot-tomcat8.5/build/tmp/javadoc/javadoc.options'

@codecov
Copy link

codecov bot commented Apr 4, 2020

Codecov Report

Merging #2649 into master will increase coverage by 0.05%.
The diff coverage is 75.43%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2649      +/-   ##
============================================
+ Coverage     73.25%   73.31%   +0.05%     
- Complexity    11193    11206      +13     
============================================
  Files           991      993       +2     
  Lines         43132    43181      +49     
  Branches       5361     5367       +6     
============================================
+ Hits          31597    31656      +59     
+ Misses         8777     8774       -3     
+ Partials       2758     2751       -7     
Impacted Files Coverage Δ Complexity Δ
...corp/armeria/spring/ArmeriaServerStartedEvent.java 50.00% <50.00%> (ø) 2.00 <2.00> (?)
...iaServerPortInfoApplicationContextInitializer.java 75.00% <75.00%> (ø) 2.00 <2.00> (?)
.../web/reactive/ArmeriaReactiveWebServerFactory.java 72.09% <83.33%> (ø) 19.00 <3.00> (ø)
...ecorp/armeria/spring/ArmeriaAutoConfiguration.java 75.47% <100.00%> (+0.47%) 11.00 <0.00> (ø)
.../linecorp/armeria/spring/web/ArmeriaWebServer.java 67.56% <100.00%> (+1.85%) 9.00 <1.00> (ø)
...eriaReactiveWebServerFactoryAutoConfiguration.java 100.00% <100.00%> (ø) 2.00 <1.00> (ø)
.../com/linecorp/armeria/server/RoutingPredicate.java 69.35% <0.00%> (-1.62%) 20.00% <0.00%> (-1.00%)
...necorp/armeria/client/HeapBasedEventLoopState.java 92.78% <0.00%> (-1.04%) 31.00% <0.00%> (-1.00%)
...rmeria/internal/client/grpc/ArmeriaClientCall.java 81.86% <0.00%> (-0.99%) 51.00% <0.00%> (-1.00%)
...java/com/linecorp/armeria/server/DefaultRoute.java 94.73% <0.00%> (-0.66%) 71.00% <0.00%> (-1.00%)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 145de6e...3717e27. Read the comment docs.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Nice work, @heowc! 👍

@heowc
Copy link
Contributor Author

heowc commented Apr 6, 2020

Thanks for review ~ :-)

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Just some nits. Nice!

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Just some nits

@trustin
Copy link
Member

trustin commented Apr 16, 2020

After playing a little bit with this pull request by myself, I realized it will be very useful if a user can specify a list of SessionProtocols with the annotations, e.g. @LocalArmeriaPort(SessionProtocol.HTTP) to get HTTP port number rather than HTTPS port number. What do you think, @heowc ?

@heowc
Copy link
Contributor Author

heowc commented Apr 16, 2020

After playing a little bit with this pull request by myself, I realized it will be very useful if a user can specify a list of SessionProtocols with the annotations, e.g. @LocalArmeriaPort(SessionProtocol.HTTP) to get HTTP port number rather than HTTPS port number. What do you think, @heowc ?

Oh! good idea. However, this seems to require some changes. I'll check it. 🤔

@heowc
Copy link
Contributor Author

heowc commented Apr 17, 2020

@trustin ,

I used @Value to make @LocalArmeriaPort(s). There seems to be no way to dynamically handle @Value by adding an option.

I think the core classes inside Spring to operate @Value are AutowiredAnnotationBeanPostProcessor and QualifierAnnotationAutowireCandidateResolver, and I think you need to create separate BeanPostProcessor and AutowireCandidateResolver for @LocalArmeriaPort(SessionProtocol.HTTP) to work.

I am not sure if I should do this. what do you think? 🤔

@trustin
Copy link
Member

trustin commented Apr 20, 2020

I am not sure if I should do this. what do you think? 🤔

It'd be awesome if you could write a BeanPostProcessor or whatever required to make @LocalArmeriaPort(SessionProtocol) work. 🙇

@trustin
Copy link
Member

trustin commented Apr 24, 2020

Thanks for your patience, @heowc 🙇‍♂️

@heowc
Copy link
Contributor Author

heowc commented Apr 24, 2020

This seems to change a lot differently from the first thought. Therefore, I will close this PR and create a new one.

@heowc heowc closed this Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support @LocalArmeriaPort, @LocalArmeriaPorts
3 participants