feat: gRPC stream connection deadline #999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Guido Breitenhuber <guido.breitenhuber@dynatrace.com>
github-actions
bot
requested review from
beeme1mr,
Kavindu-Dodan,
thisthat and
toddbaert
aepfli
reviewed
Oct 3, 2024
| @@ -55,6 +56,7 @@ public GrpcStreamConnector(final FlagdOptions options) { | |||
| serviceStub = FlagSyncServiceGrpc.newStub(channel); | |||
| serviceBlockingStub = FlagSyncServiceGrpc.newBlockingStub(channel); | |||
| deadline = options.getDeadline(); | |||
| streamDeadlineMs = options.getStreamDeadlineMs(); | |||
There was a problem hiding this comment.
[suggestion] Cant we just use the options within this class, and pass it further down the call chain? - adding a new field and parameters might be tedious over time
aepfli
reviewed
Oct 3, 2024
| @@ -64,6 +65,7 @@ public GrpcConnector(final FlagdOptions options, final Cache cache, final Suppli | |||
| this.startEventStreamRetryBackoff = options.getRetryBackoffMs(); | |||
| this.eventStreamRetryBackoff = options.getRetryBackoffMs(); | |||
| this.deadline = options.getDeadline(); | |||
| this.streamDeadlineMs = options.getStreamDeadlineMs(); | |||
There was a problem hiding this comment.
[suggestion] using the options object instead of separate fields would reduce this over head of adding a new field all the time
There was a problem hiding this comment.
I can go either way on this one. In fact it might be a good thing to do in a pure refactor/cleanup PR. There's also some naming that we can probably improve with the provider.
aepfli
approved these changes
Oct 3, 2024
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
changed the title
toddbaert
reviewed
Oct 4, 2024
providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/FlagdOptions.java
Show resolved
Hide resolved
…s/flagd/FlagdOptions.java Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
approved these changes
Oct 4, 2024
toddbaert
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
adds deadlines to GRPC stream connections in the flagd provider.
In GRPC setups, where L7 proxies route traffic between the provider and flagd, broken connections in the line can cause a provider never to get updates of chaged flag configurations. The provider never recognizes broken e2e connections because it only listens on the RPC stream and never sends data.
This feature forces a client to open a new GRPC stream after the deadline is exceeded.
Notes
Default stream deadline is 10 Minutes. Deadlines can be disabled by setting it to
0.