fix(flagd): configurable retry backoff after each sync cycle error (#756)#806
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
4762cab to
9903081
Compare
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
…t behaviour to Java flagd provider Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
9903081 to
dc1cf05
Compare
aepfli
left a comment
There was a problem hiding this comment.
I am still not the biggest fan of a sleep, as this will block everything. even if do some kind of context cancellation and want to stop execution. we are stopped by a pretty long Sleep. Hence i think we should really try to use a different construct than a sleep.
The rest of the pull request looks good to me, and is ready to merge.
Disclaimer: For me the sleep is a reason to not approving this, but not a big enough reason to block this pull request from getting merged, if the rest of the approvers are fine with this.
Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
| } | ||
|
|
||
| // WithRetryBackoffMs sets the initial backoff duration (in milliseconds) for retrying failed connections | ||
| func WithRetryBackoffMs(retryBackoffMs int) ProviderOption { |
There was a problem hiding this comment.
A parameter that represents a duration should be of type time.Duration. The name of the parameter should be changed to remove any reference to a specific unit like milliseconds.
| } | ||
| } | ||
|
|
||
| cfg.RetryGracePeriod = getIntFromEnvVarOrDefault(flagdGracePeriodVariableName, defaultGracePeriod, cfg.log) |
Ya, personally I'm fine with it. We already have the same pattern in Java, so I would go with it for now. This sleep is only during what's already an unusual error scenario (connection is healthy, but stream keeps returning errors immediately), within a retry loop, so I think it's acceptable.... but looks like @alexandraoberaigner already used a timer. |
toddbaert
left a comment
There was a problem hiding this comment.
I added a recommended comment for why we are doing our own weird simple backoff for unusual situations. I also agree with @sahidvelji 's duration comment.
Otherwise LGTM.
Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com>
|
@toddbaert @sahidvelji I looked into this a bit & would like to change to |
|
I created #816 |
…pen-feature#756) (open-feature#806) Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com> Signed-off-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: leoff00 <leof0180@gmail.com>
…pen-feature#756) (open-feature#806) Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com> Signed-off-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: leoff00 <leof0180@gmail.com>
This PR
adds configurable retry backoff options to the flagd in-process provider, which allows to configure the retry policy of gRPC connections.
Aims to make the implementation consistent with Java. See this change to prevent tight loops when the retry policy doesn't take effect
Related Issues
Details
Configuration and API Enhancements:
RetryBackoffMsandRetryBackoffMaxMstoProviderConfiguration, with corresponding default values, environment variable support, and provider options (WithRetryBackoffMs,WithRetryBackoffMaxMs). These allow users to configure retry backoff durations for connection retries. [1] [2] [3] [4] [5] [6]Retry Logic and Policy:
InitialBackoffandMaxBackoffinstead of hardcoded durations, making retry behavior fully configurable.Testing and Validation:
TestBuildRetryPolicy) for the retry policy builder, verifying that the generated JSON policy reflects the configured backoff values and other retry parameters.