feat: add initialize and shutdown behavior #456
Conversation
- adds call to provider's `initialize()` method when provider is changed Signed-off-by: Lars Opitz <[email protected]>
- adds call to provider's `shutdown()` method when provider is changed Signed-off-by: Lars Opitz <[email protected]>
- fixes checkstyle issue Signed-off-by: Lars Opitz <[email protected]>
- fixes javadoc issue Signed-off-by: Lars Opitz <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #456 +/- ##
============================================
+ Coverage 93.20% 94.09% +0.89%
- Complexity 220 257 +37
============================================
Files 23 24 +1
Lines 500 576 +76
Branches 32 35 +3
============================================
+ Hits 466 542 +76
Misses 17 17
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
- cleans up test Signed-off-by: Lars Opitz <[email protected]>
- fixes test Signed-off-by: Lars Opitz <[email protected]>
|
I think we need a API shutdown method: as specified in 1.6.1. This method would basically just call |
src/test/java/dev/openfeature/sdk/testutils/FeatureProviderTestUtils.java
Show resolved
Hide resolved
|
This looks good to me. I think this needs to be added. Though it could be done in a separate PR, it's related-enough to be worth implementing here. The testing looks good. I'd like your insight on this. The same thing came up in the JS-SDK implementation and I'm wondering if we don't need to slightly tighten the spec requirement here. Not a blocker for this PR but very interested in your opinion. |
|
@open-feature/sdk-java-approvers @open-feature/sdk-java-maintainers |
toddbaert
requested review from
justinabrahms,
thisthat,
thiyagu06 and
Kavindu-Dodan
- makes API using old provider while initialization of replacement not completed Signed-off-by: Lars Opitz <[email protected]>
- fixes checkstyle issues Signed-off-by: Lars Opitz <[email protected]>
- doesn't call shutdown of replaced provider if it is bound multiple times Signed-off-by: Lars Opitz <[email protected]>
…ure#457) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Signed-off-by: Lars Opitz <[email protected]>
|
I addressed the comments of the PR:
The API level shutdown method is not yet implemented, as Spec PR#193 is not yet implemented. |
|
@toddbaert , how can i retrigger the build. it failed, as it didn't find the api key for codecov for whatever reason... 🤔 |
Retriggered and seems a GH glitch |
|
Had an initial look and implementation and especially the tests are inspiring to review :) Left one remark to follow-up |
Ya it's very annoying but sometimes when codecov gets busy it throws this error, which is a total red herring. @Kavindu-Dodan @lopitz |
- moves handling of providers out of OpenFeatureAPI into dedicated ProviderRepository - cleans up temp map for named providers/member for default provider on successful initialization - prepares API shutdown method by creating one in the Provider Repository Signed-off-by: Lars Opitz <[email protected]>
- simplifies code - increases coverage Signed-off-by: Lars Opitz <[email protected]>
- removes faulty locking code - clears all named providers on shutdown - sets NoOpProvider on shutdown Signed-off-by: Lars Opitz <[email protected]>
- avoids calling initialize on already initialized providers Signed-off-by: Lars Opitz <[email protected]>
|
I think this is good to merge after this, and considering @Kavindu-Dodan 's optional suggestions. |
Signed-off-by: Lars Opitz <[email protected]>
|
@toddbaert, @Kavindu-Dodan i addressed the comments, pls re-review |
| /** | ||
| * This method is only here for testing as otherwise all tests after the API shutdown test would fail. | ||
| */ | ||
| final void resetProviderRepository() { |
There was a problem hiding this comment.
fine with me since it's package private.
|
Kudos, SonarCloud Quality Gate passed!
|
Add initialize and shutdown behavior
This PR adds the behavior of the feature provider mutator of the OpenFeatureAPI class to call
initialize()andshutdown()of the set/replaced feature provider. It provides the implementation for the requirements 1.1.2.2 and 1.1.2.3.Related Issues
Fixes #449
Notes
I chose option 3 as described in #449, the asynchronous option to be implemented. This makes sure, there are no nasty delays in the application startup.
I also chose to provider empty default implementations for
initialize()andshutdown()in the FeatureProvider interface, to be as backward compatible as possible.Follow-up Tasks
This PR doesn't implement the client events as described in #440. Depending on the implementation, the tests might need be changed. Hence, I created a helper class
FeatureProviderTestUtilswhich encapsulates the new async nature of the provider mutator.How to test
Unit and spec tests are provided.