Skip to content

chore(flagd): refactoring codebase into multiple classes #378

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Aug 4, 2023
Merged

chore(flagd): refactoring codebase into multiple classes #378

merged 11 commits into from
Aug 4, 2023

Conversation

thisthat
Copy link
Member

@thisthat thisthat commented Jul 28, 2023 •
edited
Loading

This PR

  • refactor the codebase into more packages and classes

Related Issues

Fixes #365

@thisthat thisthat changed the title move resolving classes chore(flagd): refactoring codebase into multiple packages Jul 28, 2023
@thisthat thisthat changed the title chore(flagd): refactoring codebase into multiple packages chore(flagd): refactoring codebase into multiple classes Jul 28, 2023
thisthat
thisthat commented Aug 3, 2023
View reviewed changes
...rs/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/grpc/EventStreamObserver.java
@@ -46,25 +55,20 @@ public void onError(Throwable t) {
if (this.cache.getEnabled()) {
this.cache.clear();
Copy link
Member Author

@thisthat thisthat Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should not bust the cache in case of error and instead use the latest known values until the stream is reconnected. This line basically prevents to do ANY flag evaluation if we cannot connect to flagd. IMHO we should still be able to resolve flags according to what we have in the cache instead of failing the gRPC resolution and use the default value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to #390

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that a connection lost results in stale flags. And stale flags could result in inconsistent behavior. So I think we should keep this.

However, another option is to make this a configurable option

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is we don't cache all values, we only cache static flags (flags without rules).

This puts us in a situation where we really can't evaluate all flags. If we were evaluating flags in memory, we could just have "STALE" flags, but with the RPC design, we simply can't evaluate dynamic flags at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This screams for a configurable option as @Kavindu-Dodan suggests 🤔
I'll try to explain my thoughts: since values are static, I want to "remotely rewrite" the default value in the code. Hence, saving the last known values would allow me to evaluate things as the last point in time I had a connection to flagd.
For dynamic flags, failing and defaulting to the default value is fine since I cannot pass the ctx to flagd to evaluate the flags according to the rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how some people might be interested in this option. It's worth discussing, but it can't be done in isolation. We can talk about it (perhaps in a flagd discussion?) and add it to the doc so it can be implemented/scheduled in all flagd RPC providers.

@thisthat thisthat mentioned this pull request Aug 3, 2023
Closed
@thisthat thisthat marked this pull request as ready for review August 3, 2023 11:30
@thisthat thisthat requested a review from a team as a code owner August 3, 2023 11:30
@thisthat thisthat force-pushed the chore/365/refactor-pkgs branch from cbcfba4 to 7227c2a Compare August 3, 2023 11:31
thisthat added 5 commits August 3, 2023 17:04
@thisthat
move resolving classes
0c90b07 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
move pkgs
e852780 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
move test pkg
24d6bd2 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
add smoke test
dc628ef 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
Refactor code
56c4ec1 
Signed-off-by: Giovanni Liva <[email protected]>
thisthat and others added 5 commits August 3, 2023 17:04
@thisthat
move resolution logic into its own class
d9ab184 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
fix tests
37eb5c3 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
bit of cleanup
5cc0db6 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
more refactoring
c2ef8e3 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat
remove smoketests in favor of e2e tests
6be8c10 
Signed-off-by: Giovanni Liva <[email protected]>
@thisthat thisthat force-pushed the chore/365/refactor-pkgs branch from 7227c2a to 6be8c10 Compare August 3, 2023 15:05
Kavindu-Dodan
Kavindu-Dodan reviewed Aug 3, 2023
View reviewed changes
Kavindu-Dodan
Kavindu-Dodan approved these changes Aug 3, 2023
View reviewed changes
Copy link
Collaborator

@Kavindu-Dodan Kavindu-Dodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with stated comments and fixes

@thisthat
addressing review
539700e 
Signed-off-by: Giovanni Liva <[email protected]>
@toddbaert toddbaert self-requested a review August 3, 2023 20:31
@toddbaert
Copy link
Member

Thanks @thisthat ! Looks much better IMO.

@toddbaert toddbaert merged commit 0a73bbc into open-feature:main Aug 4, 2023
@toddbaert toddbaert mentioned this pull request Aug 16, 2023
Merged
DBlanchard88 pushed a commit to DBlanchard88/java-sdk-contrib that referenced this pull request Apr 29, 2024
@renovate
chore(deps): update github/codeql-action digest to f32426b (open-feat…
ae30789 
…ure#378)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddbaert toddbaert toddbaert approved these changes

@Kavindu-Dodan Kavindu-Dodan Kavindu-Dodan approved these changes

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

Assignees

@beeme1mr beeme1mr

@Kavindu-Dodan Kavindu-Dodan

@toddbaert toddbaert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FlagD] Refactor package structure and offload some responsibilities from FlagdProvider
4 participants
@thisthat @toddbaert @Kavindu-Dodan @beeme1mr