refactor: add support for client timeouts#217
Conversation
| public class ClientConfig { | ||
| Duration timeout; | ||
|
|
||
| public static ClientConfig DEFAULT = new ClientConfig(Duration.ofSeconds(5)); |
There was a problem hiding this comment.
Default looks good,
max for this as observed in past is <5s
There was a problem hiding this comment.
nit: we can add a TODO here to read this value POJO from config
There was a problem hiding this comment.
Because this is used by so many different services, I don't love providing it from config - since that means the config itself is needed to build the abstract store object which means every service now needs to get access to it and pass it in. The override case certainly could be from config, but let me think through how/if we could accomplish this.
There was a problem hiding this comment.
Added a todo, and moved the default up to 10s to make sure we're safe (since the intention here is to be as compatible with existing behavior as possible we don't even want to approach the 5s limit)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
============================================
+ Coverage 79.92% 80.02% +0.09%
- Complexity 488 496 +8
============================================
Files 54 55 +1
Lines 2416 2428 +12
Branches 106 107 +1
============================================
+ Hits 1931 1943 +12
+ Misses 425 424 -1
- Partials 60 61 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3 participants
Description
This change adds support for client timeouts on all config store abstractions, using a default of 5s if unset.