Proof of concept for Visitor-pattern-based handling of Responses#287
Proof of concept for Visitor-pattern-based handling of Responses#287uschi2000 wants to merge 1 commit into
Conversation
|
NetflixOSS » feign » feign-pull-requests #170 FAILURE |
There was a problem hiding this comment.
Maybe add a boolean handles(Response response, MethodMetadata metadata) and have handle return an Object.
|
My suggestion would be to have a look at the hystrix support. If we added a plugin arch, it should be able to address existing use cases of extensibility. IOTW, we don't really want to do this twice and I think the hystrix code merged recently probably shows how to achieve your concerns. If it doesn't, it would be good to note the gaps. https://github.com/Netflix/feign/tree/master/hystrix |
|
Cheers, will have a look. |
|
@adriancole , I may not understand right what you mean. When I look at the implementation of the Hystrix module, it looks like it's wrapping the reflection proxy in order to wrap the value returned from remote into a new HystrixCommand. This technique doesn't quite work for the my case described above since I need to change the way the MethodHandler itself works; seeing it's return value is not enough. Maybe I'm missing something? |
|
I guess what I was thinking about is that you are looking to supply a fallback value (ex absent on 404). This requires coordination in the same way hystrix does. Hystrix has a richer fallback system, so if anything I would expect more code there. For example, to acheive the opinion you want, you are teaching feign optional (codec) and also how to interpret exceptions (ex 404 should fallback to absent is possible today with an invocation handler). Coordinating multiple aspects is what hystrix is doing as well. By first using the least code, we can understand how much pain this causes and how this leads to a cure. I'm hesitant to add concepts that don't also increase the clarity of similar code such as hystrix which also handles fallbacks. Did this explanation help or hurt? |
|
@adriancole thanks for your suggestions. An implementation analogous to the Hystrix one looks roughly like this: A few things feel a little off about this:
Bottom-line: I can run with the above work-around for now, but would love to see a more first-class way of allowing users to configure response decoding. Happy to help out with code as well, let me know. |
|
Thanks for the input! @spencergibb @jdamick any other ideas on this? I've been highly resistant The 404 scenario is common enough that I could see us entertaining an |
|
Adrian, do I understand right that feign in your opinion should be opinionated about interpreting HTTP codes? I don't (yet) see the downside of providing sane defaults (as per the current behaviour), but allowing users to customize as they see fit. |
|
the harm is that the interface by which we support customization is
something we have to deal with for years. It is good software design to
keep narrowest possible interface to support the highest amount of use
cases. This is what feign tries to do.
Interfaces such as those addressing fallbacks often limit other types of
work. For example, we at one point did have a fallback interface. This was
removed as it wasn't compatible with async invocation.
It is the case that people ask to treat non-success range as success. I
usually offer to them alternatives, such as make a client wrapper that
returns a proper http code, when the api doesn't. Usually, they are very
edge case, and not something most are interested in.
Then, there's 404 -> some empty val. This is something I agree is worth
looking into, but not if it means opening the entire status range. It
simply doesn't require that in order to support it.
When we introduce a new interface, we have to make sure it is worth it. The
clutter of several classes to support coercing 404 -> X is overkill for the
problem.
Libraries who go for widest interface to support narrow decisions often are
just lengthening the rope to hang themselves with. They also lose the
product context, like what this tool even does. Ex. the actual requirement
is addressing 404, not all codes.
it may seem like just adding several types to support 404 isn't a problem
in isolation. It could be the kiss of death, though. Few like to debug,
answer questions, or maintain unnecessarily complex code. It is really
important to not swamp the project.
|
|
perhaps the error decoder could give the option to not throw.. it seems like if there was a ThrowingErrorDecoder vs. Non that would more simply solve this problem? |
|
Honestly, if given a choice, I prefer this to adding a new interface or a
special 404 hook. Basically, it means reverting this I think
#12
|
|
problem is that lots of folks have ErrorDecoders already.. and the
fallback capable impl is not compatible with the non.
|
|
we're chatting on gitter about this
|
|
Thanks for sharing your philosophy, Adrian. It would be great to see 404->Optional handling and I trust you will find the right way of supporting it in feign. Until then, I will go with the InvocationHandler route. |
|
ok here's the gist. I think we should introduce 404 -> something via a decoder flag. next in the long answers :) We have one pattern for integration of systems that have a special return value. I don't think 404 falls into this bucket, or needs to. If we had a way to address 404, which is semantically defined as absent, we gain a lot. Firstly, we have a fast-path for addressing absence which doesn't accidentally also catch bad request, redirect, etc. In other words, it is a very safe policy: it doesn't overlap with retries or other behavior in feign. Secondly, we don't create a cliff, where folks looking for error handling eventually realize they can't address all fallbacks with only an http response code (bc exceptions can happen far before you get a response from the server). This is a slippery slope that leads to things like Hystrix, so might as well encourage that for real errors. Most importantly, addressing 404 is extremely high value. You are just the last to mention it. For example, I would use this in denominator. Moreover, it doesn't require us to introduce or break an interface. This means all users, such as spring-cloud, can easily opt-into this. It could substantially reduce their code in the same way it can reduce your code, but without new types. |
|
Great, thanks a lot! |
This adds the `Feign.Builder.decode404()` flag which indicates decoders should process responses with 404 status. It also changes all first-party decoders (like gson) to return well-known empty values by default. Further customization is possible by wrapping or creating a custom decoder. Prior to this change, we used custom invocation handlers as the way to add fallback values based on exception or return status. `feign-hystrix` uses this to return `HystrixCommand<X>`, but the general pattern applies to anything that has a type representing both success and failure, such as `Try<X>` or `Observable<X>`. As we define it here, 404 status is not a retry or fallback policy, it is just empty semantics. By limiting Feign's special processing to 404, we gain a lot with very little supporting code. For example, we have a fast-path for addressing absence which doesn't accidentally turn bad request, redirect, silently to null. Processing 404 is a very safe policy: it doesn't overlap with retries or other behavior in feign which could turn into support questions. Also, we don't create a cliff, where folks looking for error handling eventually realize they can't address all fallback policy with only an http response code (bc exceptions can happen far before you get a response from the server). By noting 404 as a special case, we avoid a slippery slope of incrementally half-implementing Hystrix or similar. This design supports the common pattern of empty semantics with the least code on both users and the maintainers of Feign. We can still recommend other libraries such as Hystrix or Ribbon for advanced retry or fallback policy. Most importantly, addressing 404 is extremely high value. You are just the last to mention it. For example, I would use this in denominator. Moreover, it doesn't require us to introduce or break an interface. This means all users, such as spring-cloud, can easily opt-into this. It could substantially reduce their code in the same way it can reduce your code, but without new types. See #238 #287
This adds the `Feign.Builder.decode404()` flag which indicates decoders should process responses with 404 status. It also changes all first-party decoders (like gson) to return well-known empty values by default. Further customization is possible by wrapping or creating a custom decoder. Prior to this change, we used custom invocation handlers as the way to add fallback values based on exception or return status. `feign-hystrix` uses this to return `HystrixCommand<X>`, but the general pattern applies to anything that has a type representing both success and failure, such as `Try<X>` or `Observable<X>`. As we define it here, 404 status is not a retry or fallback policy, it is just empty semantics. By limiting Feign's special processing to 404, we gain a lot with very little supporting code. For example, we have a fast-path for addressing absence which doesn't accidentally turn bad request, redirect, silently to null. Processing 404 is a very safe policy: it doesn't overlap with retries or other behavior in feign which could turn into support questions. Also, we don't create a cliff, where folks looking for error handling eventually realize they can't address all fallback policy with only an http response code (bc exceptions can happen far before you get a response from the server). By noting 404 as a special case, we avoid a slippery slope of incrementally half-implementing Hystrix or similar. This design supports the common pattern of empty semantics with the least code on both users and the maintainers of Feign. We can still recommend other libraries such as Hystrix or Ribbon for advanced retry or fallback policy. See #238 #287
This adds the `Feign.Builder.decode404()` flag which indicates decoders should process responses with 404 status. It also changes all first-party decoders (like gson) to return well-known empty values by default. Further customization is possible by wrapping or creating a custom decoder. Prior to this change, we used custom invocation handlers as the way to add fallback values based on exception or return status. `feign-hystrix` uses this to return `HystrixCommand<X>`, but the general pattern applies to anything that has a type representing both success and failure, such as `Try<X>` or `Observable<X>`. As we define it here, 404 status is not a retry or fallback policy, it is just empty semantics. By limiting Feign's special processing to 404, we gain a lot with very little supporting code. If instead we opened all codes, Feign could easily turn bad request, redirect, or server errors silently to null. This sort of configuration issue is hard to troubleshoot. 404 -> empty is a very safe policy vs all codes. Moreover, we don't create a cliff, where folks seeking fallback policy eventually realize they can't if only given a response code. Fallback systems like Hystrix address exceptions that occur before or in lieu of a response. By special-casing 404, we avoid a slippery slope of half- implementing Hystrix. Finally, 404 handling has been commonly requested: it has a clear use- case, and through that value. This design supports that without breaking compatibility, or impacting existing integrations such as Hystrix or Ribbon. See #238 #287
This adds the `Feign.Builder.decode404()` flag which indicates decoders should process responses with 404 status. It also changes all first-party decoders (like gson) to return well-known empty values by default. Further customization is possible by wrapping or creating a custom decoder. Prior to this change, we used custom invocation handlers as the way to add fallback values based on exception or return status. `feign-hystrix` uses this to return `HystrixCommand<X>`, but the general pattern applies to anything that has a type representing both success and failure, such as `Try<X>` or `Observable<X>`. As we define it here, 404 status is not a retry or fallback policy, it is just empty semantics. By limiting Feign's special processing to 404, we gain a lot with very little supporting code. If instead we opened all codes, Feign could easily turn bad request, redirect, or server errors silently to null. This sort of configuration issue is hard to troubleshoot. 404 -> empty is a very safe policy vs all codes. Moreover, we don't create a cliff, where folks seeking fallback policy eventually realize they can't if only given a response code. Fallback systems like Hystrix address exceptions that occur before or in lieu of a response. By special-casing 404, we avoid a slippery slope of half- implementing Hystrix. Finally, 404 handling has been commonly requested: it has a clear use- case, and through that value. This design supports that without breaking compatibility, or impacting existing integrations such as Hystrix or Ribbon. See #238 #287
This adds the `Feign.Builder.decode404()` flag which indicates decoders should process responses with 404 status. It also changes all first-party decoders (like gson) to return well-known empty values by default. Further customization is possible by wrapping or creating a custom decoder. Prior to this change, we used custom invocation handlers as the way to add fallback values based on exception or return status. `feign-hystrix` uses this to return `HystrixCommand<X>`, but the general pattern applies to anything that has a type representing both success and failure, such as `Try<X>` or `Observable<X>`. As we define it here, 404 status is not a retry or fallback policy, it is just empty semantics. By limiting Feign's special processing to 404, we gain a lot with very little supporting code. If instead we opened all codes, Feign could easily turn bad request, redirect, or server errors silently to null. This sort of configuration issue is hard to troubleshoot. 404 -> empty is a very safe policy vs all codes. Moreover, we don't create a cliff, where folks seeking fallback policy eventually realize they can't if only given a response code. Fallback systems like Hystrix address exceptions that occur before or in lieu of a response. By special-casing 404, we avoid a slippery slope of half- implementing Hystrix. Finally, 404 handling has been commonly requested: it has a clear use- case, and through that value. This design supports that without breaking compatibility, or impacting existing integrations such as Hystrix or Ribbon. See #238 #287
Hi @adriancole ,
This pull-request is a proof-of-concept for making response handling more flexible. (Note that I haven't fixed up the tests, so it doesn't compile.) The motivation is as follows: Our servers often return 404s for non-existing resources and we'd like to surface those as Optional#absent (Guava or Java8) in clients, rather than as exceptions. Given the current architecture of response handling in SynchronousResponseHandler this is not possible since the configured Decoder is only invoked for [200, 300) error codes. This PR shows how to use a Visitor pattern to make response handling more flexible and configurable.
I'd like to know if you'd in principle be happy to consider this or a similar approach, or if you are opposed based on some fundamental consideration. In the former case, I'm happy to get APIs and tests in good shape according to your suggestions. Back-compat of the Feign.Builder class will be an issue.
Cheers!
Robert