-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FB-279: Sync FeatureBoard API state back to ExternalState provider if registered #79
base: main
Are you sure you want to change the base?
Conversation
} | ||
catch (ArgumentException e) // eg. thrown due to duplicate feature key | ||
{ | ||
_logger.LogError(e, "Failed to update flags"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the eTag value be rolled back to its original value if exception occurs at this point (ie. during handling of the FeatureConfigurationUpdated
event)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not hugely experienced in handling eTags, but isn't just using a new value fine since you are trying to compare tag values in the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the eTag retains its updated value and isn't rolled back then the code inside try block won't run again until FeatureBoard API responds back with a non 304 status (due to someone touching the feature config).
I think, at least for this exception scenario [caused by duplicate feature name appearing in FB API response), rollback is appropriate, so it keeps retrying the state update periodically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tricker question is what to do if IFeatureBoardExternalState
throws
e22d4c4
to
171826b
Compare
await scope.ServiceProvider.GetRequiredService<IFeatureBoardService>().RefreshFeatureConfiguration(cancellationToken); | ||
using var scope = _scopeFactory.CreateScope(); | ||
var updated = await scope.ServiceProvider.GetRequiredService<IFeatureBoardService>().RefreshFeatureConfiguration(cancellationToken) ?? false; | ||
if (updated || _externalState is null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah cool
171826b
to
f475ddc
Compare
c4e6c63
to
da09dd0
Compare
…gnal as async exception behavhiour is not fit for purpose
da09dd0
to
fa68f9e
Compare
Relevant JIRAs: FB-279 & FB-246
Summary of changes:
Retry-After
header is present in response