Conversation
pkoenig10
commented
Mar 26, 2026
Comment on lines
-136
to
-139
| // Assert that timeouts are in milliseconds, not any higher precision, because feign only supports millis. | ||
| checkTimeoutPrecision(connectTimeout(), "connectTimeout"); | ||
| checkTimeoutPrecision(readTimeout(), "readTimeout"); | ||
| checkTimeoutPrecision(writeTimeout(), "writeTimeout"); |
Member
Author
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)); |
Member
Author
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 endpoint that still use Jersey.
Comment on lines
-260
to
-261
| // Note that Feign overrides OkHttp timeouts with the timeouts given in FeignBuilder#Options if given, or | ||
| // with its own default otherwise. |
Member
Author
There was a problem hiding this comment.
No longer correct, for the reason mentioned above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.