search

opentable / otj-server

OpenTable Java server template component
Apache License 2.0
1 stars 16 forks source link

@ReactiveServer using otj-jackson configuration #125

Closed dkaukov closed 4 years ago

mikebell90 commented 4 years ago

This looks good to me, but @scottjohnson needs to sign off on it, and consider the compatibility implications.

Scott the context of this PR is Dmitry showed the WebFlux server did NOT use otj-jackson objectmapper.

scottjohnson commented 4 years ago

I agree with the general issue: We have to somehow explicitly load the object mapper in to the server, which we assumed would happen before but didn't.

The issue I see with this solution is that WebFlux has multiple different entry points for how to configure the server. My original delivery of the @ReactiveServer annotation was an attempt to let our platform continue to support both first-class configuration approaches. Unfortunately the changes in this PR only work for one of those two approaches. I think we need to figure out how to make this work with both.

scottjohnson commented 4 years ago

I pushed a commit that re-works ReactiveServerJacksonConfiguration to work in a manner that is compatible with the @ReactiveServer/@EnableWebFlux/DelegatingWebFluxConfiguration config approach.

@dkaukov can you confirm if this approach works in your spring boot auto config based services as well?

Aside, @mikebell90 I noticed that otj-server-core tests are failing sporadically with the following error:

[ERROR] com.opentable.server.PortSelectionWithNamedPortsAndAClashInKubernetesTest.testPortSelection  Time elapsed: 0.019 s  <<< ERROR!
java.lang.IllegalStateException: Failed to load ApplicationContext
Caused by: org.springframework.boot.web.server.PortInUseException: Port 8080 is already in use

If looks like some previous K8s related change is causing parallel tests to all spin up with a server on port 8080 simultaneously.

dkaukov commented 4 years ago

@scottjohnson LGTM @mikebell90 I'll take a look on the test in context of separate PR

scottjohnson commented 4 years ago

This can be merged. We'll discuss the release.