More robust sync mechanism #63
Conversation
client.go
Outdated
| @@ -92,9 +96,17 @@ func (c KeywhizHTTPClient) Logger() *logrus.Entry { | |||
|
|
|||
| // NewClient produces a read-to-use client struct given PEM-encoded certificate file, key file, and | |||
There was a problem hiding this comment.
nit: this comment should be updated to reflect the change in arguments
mbyczkowski
force-pushed
the
robust-sync
branch
from
6bf4ae6
to
0b1b792
Compare
client.go
Outdated
| for i := 0; i < c.params.maxRetries; i++ { | ||
| now := time.Now() | ||
| resp, err = c.httpClient.Get(t.String()) | ||
| if err != nil || resp.StatusCode == http.StatusOK { |
There was a problem hiding this comment.
I'm wondering if there's any other status codes we shouldn't retry for. 404 maybe.
There was a problem hiding this comment.
probably 401, 403 and generally most (if not all 4xx)? I think we should mainly care about 5xx. I will pull this into isIntermittent method
config.go
Outdated
| } | ||
|
|
||
| if config.MaxBackoff == "" { | ||
| config.MaxBackoff = "10000ms" |
There was a problem hiding this comment.
nit: "10s" is easier to read
config.go
Outdated
| MaxRetries uint16 `yaml:"max_retries"` // Optional: retry each HTTP call after non-200 response. Defaults to Keysync's config. | ||
| Timeout string `yaml:"timeout"` // Optional: timeout HTTP calls. Defaults to Keysync's config. | ||
| MinBackoff string `yaml:"min_backoff"` // Optional: wait time before first retry. Defaults to Keysync's config. | ||
| MaxBackoff string `yaml:"max_backoff"` // Optional: max wait time before retries. Defaults to Keysync's config. |
There was a problem hiding this comment.
I don't think we need to be able to configure backoff per-client, and it's just extra complexity. We don't allow configuring servers per-client, and it makes sense to configure retries and backoff with the server.
There was a problem hiding this comment.
cool, will simplify 👍
There was a problem hiding this comment.
@mcpherrinm makes sense, I used ConfigClient to refactor NewClient, so I can just make those vars be not readable/settable from YAML client config and just copy them from Config.
mbyczkowski
force-pushed
the
robust-sync
branch
from
0b1b792
to
1418b07
Compare
Retries can be specified in the config with `max_retries`. Defaults to 1.
mbyczkowski
force-pushed
the
robust-sync
branch
from
1418b07
to
e18f5dc
Compare
MMMMretries, min backoff time and max backoff can be specified in main keysync config with overrides available for each client. The config takes a string and later attemps to parse the duration using `time.ParseDuration` - this behavior is analogous to `poll_interval`. As part of the refactor, the timeout value is now configurable (defaults to 60s in absence of keysync's config) and also can be overridden by each client.
mbyczkowski
force-pushed
the
robust-sync
branch
from
4489a3f
to
e2f1621
Compare
This PR should address issues discussed in #30.
TL;DR: This PR introduces retries with exponential backoff (+ jitter for spreading the load) for each client sync API call
Thoughts/questions:
I'm still pondering the best config representation for the backoff, and I'm leaning towards using strings andtime.ParseDuration. I've added some simple validation checks for the config, so would be easy to get this changed and handle potential errors.Timeout,MinBackoffandMaxBackoffcan now be specified in the same way asPollInterval(so20s,100msetc.)hopefullythat's OK.