Conversation
| // Assert that timeouts are in milliseconds, not any higher precision, because feign only supports millis. | ||
| checkTimeoutPrecision(connectTimeout(), "connectTimeout"); | ||
| checkTimeoutPrecision(readTimeout(), "readTimeout"); | ||
| checkTimeoutPrecision(writeTimeout(), "writeTimeout"); |
There was a problem hiding this comment.
This is no longer relevant. Since #1596 we no longer use these values to configure the Feign Options. We no longer use the default Feign client (which respects these options) and instead use our Dialogue-based client, which ignores them (because this is handled by the underlying Dialogue client).
| // Exception mappers | ||
| context.register(new NoContentExceptionMapper()); | ||
| context.register(new IllegalArgumentExceptionMapper(exceptionListener)); | ||
| context.register(new RetryableExceptionMapper(exceptionListener)); |
There was a problem hiding this comment.
This isn't providing any value. Without this, these errors will just get handled by RuntimeExceptionMapper, which has identical behavior.
Not to mention that:
- Most client errors should be handled inside the Dialogue client and not make it to the Feign wrapper where they may be wrapped in
RetryableExceptionMapper. - We have relatively few endpoints that still use Jersey.
| // Note that Feign overrides OkHttp timeouts with the timeouts given in FeignBuilder#Options if given, or | ||
| // with its own default otherwise. |
There was a problem hiding this comment.
No longer correct, for the reason mentioned above.
iamdanfox
left a comment
iamdanfox
left a comment
There was a problem hiding this comment.
I'm supportive of this because severing the compile dependency from conjure-java-jersey-jakarta-server -> com.netflix.feign:feign-core means that I think I can safely exclude this old feign-core version
2 participants
conjure-java-runtimehas a dependency on an old, unmaintained version of Feign. This is causing classpath conflicts with consumers are using a modern version of Feign, as the package has changed toio.github.openfeign:feign-coreand the library has gone through a couple major version changes.This PR is a small first step that eliminates our Feign dependency in a couple places where it is no longer needed.
After this PR,
conjure-java-jaxrs-clientis the only project that still has a Feign dependency. In a follow-up, I'd like to fork and vendor Feign as a private implementation detail of this project.