search

paypal / mocca

Mocca is a GraphQL client for JVM languages with the goal of being easy to use, flexible and modular.
MIT License
15 stars 14 forks source link

Fix timeout support #41 #43

Closed crankydillo closed 2 years ago

crankydillo commented 2 years ago

See the issue for more info, but basically the code I'm replacing assumed that:

  1. Connect/read timeout was always done with client creation.
  2. Our impl (technically feign) couldn't successfully set those per request.

This work addresses the above and 3 issues:

  1. How a client can specify how the timeouts are specified.
  2. How the builder chain can honor the above in a nice way (i.e if you a client says it doesn't support request timeouts then those aren't exposed via the build chain).
  3. How to test without repetition.

So, it was just interesting to try and solve this. Now, I don't think this code is super-complicated, but I can understand distaste for this. I primarily did it just out of interest:) I think you could make a decent argument that we just bring back the timeout methods to all builder chains and in the jaxrs2+client, we'd just log that these are ignored.
That's unfortunate, but if this is the only case where we need to ignore request-level timeouts, then I can understand not wanting this code.

Even if we get rid of this, there should still be some thought to whether we do timeout tests. I lean towards no..

crankydillo commented 2 years ago

@fabiocarvalho777 I think we'll have to find another mechanism other than deleting develop during a release. I don't think we want to close all open PRs:)