feat(flagd): introduce fatalStatusCodes option #1624
Conversation
github-actions
bot
requested review from
Kavindu-Dodan,
beeme1mr,
thisthat and
toddbaert
...lagd/src/test/java/dev/openfeature/contrib/providers/flagd/e2e/steps/config/ConfigSteps.java
Outdated
Show resolved
Hide resolved
...ers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/e2e/steps/ProviderSteps.java
Outdated
Show resolved
Hide resolved
providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/e2e/steps/Utils.java
Outdated
Show resolved
Hide resolved
providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/FlagdOptions.java
Outdated
Show resolved
Hide resolved
leakonvalinka
force-pushed
the
fix/flagd-infinite-connection-retries
branch
from
6f89ff0 to
057751b
Compare
Signed-off-by: lea konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: lea konvalinka <lea.konvalinka@dynatrace.com>
leakonvalinka
force-pushed
the
fix/flagd-infinite-connection-retries
branch
from
f7f1d97 to
f0a1db2
Compare
Signed-off-by: lea konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: lea konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: Konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: lea konvalinka <lea.konvalinka@dynatrace.com>
leakonvalinka
changed the title
Signed-off-by: Konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: Konvalinka <lea.konvalinka@dynatrace.com>
chrfwow
left a comment
chrfwow
left a comment
There was a problem hiding this comment.
Since we do not want to introduce breaking changes into the api by adding a PROVIDER_FATAL type to ProviderEvent, I have two suggestions how we might be able to work around the "misuse" of the stale event:
We could add a isFatal flag to the FlagdProviderEvent to track the type of error. I don't really like it because this could also be set when the event is not an error event, and with this we split up information that should be stored in one place into two places.
Or, we create an enum class ExtendedProviderEvent, which is a copy of ProviderEvent (enums cannot be extended in Java), plus the additional PROVIDER_FATAL field. We would then have to map where needed between the two types (not 100% sure if this will work). I don't like this either, because we would duplicate the ProviderEvent enum
| private final BlockingQueue<QueuePayload> outgoingQueue = new LinkedBlockingQueue<>(QUEUE_SIZE); | ||
| private final FlagSyncServiceStub flagSyncStub; | ||
| private final FlagSyncServiceBlockingStub metadataStub; | ||
| private final List<String> fatalStatusCodes; |
There was a problem hiding this comment.
Since we do lots of .contains operation on this data structure, a HashSet might be more performant. How many entries do we expect in this list?
There was a problem hiding this comment.
That's hard for me to estimate, what do the others think? The currently defined default is an empty list
...rs/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/rpc/RpcResolver.java
Show resolved
Hide resolved
| .map(String::trim) | ||
| .collect(Collectors.toList()) : defaultValue; | ||
| } catch (Exception e) { | ||
| return defaultValue; |
There was a problem hiding this comment.
We should print an info/warn that the env vars are invalid
There was a problem hiding this comment.
Just for this method? Or the other ones too? I'd either leave it or add it in all cases to be consistent
There was a problem hiding this comment.
Then we should add it everywhere, but in a different PR
There was a problem hiding this comment.
Alright, sounds good. Should we create a new issue for this or is that overkill?
| * Defaults to empty list | ||
| */ | ||
| @Builder.Default | ||
| private List<String> fatalStatusCodes = fallBackToEnvOrDefaultList(Config.FATAL_STATUS_CODES_ENV_VAR_NAME, List.of()); |
There was a problem hiding this comment.
Do we really want to retry on every error code per default? How is this handled in our other sdks?
There was a problem hiding this comment.
No, you're right, I'll rephrase it to "for which the provider transitions into fatal mode upon first connection". The general retry policy is defined here and is the same for all sdks afaik
| if (syncResources.getPreviousEvent() != ProviderEvent.PROVIDER_ERROR) { | ||
| onError(); | ||
| syncResources.setPreviousEvent(ProviderEvent.PROVIDER_ERROR); | ||
| case PROVIDER_STALE: |
There was a problem hiding this comment.
Please update the javadoc above the switch, we do now use the STALE state
There was a problem hiding this comment.
Will do once we agree on a final implementation
This is an implication of our provider design and there is not really something to do about that (in this PR). |
8 participants
This PR
Related Issues
resulted from this issue
Notes
I'm not too happy with how the fatal error is communicated through the different components (received at
SyncStreamQueueSource->FlagStore->InProcessResolver->FlagdProvider, respectiveRpcResolver->FlagdProvider). It "misuses" the STALE state to differentiate between normal errors and fatal errors. I couldn't find a cleaner solution for this unfortunately, so feedback on this would be highly appreciated!Will work on the remaining failing tests once we agree on how to proceed!
Follow-up Tasks
How to test