Skip to content

chore: refactor flagd event handling #398

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 15 commits into from
Aug 17, 2023
Merged

chore: refactor flagd event handling #398

merged 15 commits into from
Aug 17, 2023

Conversation

Kavindu-Dodan
Copy link
Collaborator

@Kavindu-Dodan Kavindu-Dodan commented Aug 15, 2023 •
edited
Loading

This PR

Completes some of the tasks mentioned at #390

Introduce clean contracts for flagd evaluator.

  • Introduce abstraction layer between flagd OF implementation & GRPC connection handling - Resolver interface
  • Remove test focused constructors and rely on reflection to inject mocks
  • Retry logic is now handled by a dedicated thread & utilize wait notify mechanism. Dummy calls were removed
  • Shutdown is propagated to the retry logic handler (thread) to allow proper resource celanup

Best reviewed either checking out the code OR visiting branch directly on my fork - https://github.com/Kavindu-Dodan/java-sdk-contrib/tree/feat/refactor-flagd/providers/flagd

@Kavindu-Dodan Kavindu-Dodan requested a review from a team as a code owner August 15, 2023 21:23
@github-actions github-actions bot requested review from beeme1mr and toddbaert August 15, 2023 21:23
@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/refactor-flagd branch 2 times, most recently from f002301 to 2b0fefb Compare August 15, 2023 21:41
@thisthat
Copy link
Member

Please, change the wording in the PR description from Fixes to Part of or something similar. Otherwise the original issue will be closed when not all the points are addressed

liran2000
liran2000 reviewed Aug 16, 2023
View reviewed changes
Copy link
Member

@liran2000 liran2000 left a comment

Choose a reason for hiding this comment

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

Hi, added some comments, hope it's ok.

@Kavindu-Dodan
Copy link
Collaborator Author

Kavindu-Dodan commented Aug 16, 2023 •
edited
Loading

Please, change the wording in the PR description from Fixes to Part of or something similar. Otherwise the original issue will be closed when not all the points are addressed

Done. However, we can mark Use always OTel interceptor can be considered done with the new packaging structure

@Kavindu-Dodan
Copy link
Collaborator Author

@thisthat @toddbaert @liran2000 thank you for the reviews 🙌

I addressed all your concerns. As summary,

  • Retry logic now solely rely on the eventing mechanism (no dummy calls)
  • Graceful shutdowns for event listener implemented - proper resource cleanup
  • Justification on thread usage along with proper log levels and comments where necessary

@liran2000
Copy link
Member

@Kavindu-Dodan thanks :) had one question above.

@toddbaert
Copy link
Member

@thisthat @toddbaert @liran2000 thank you for the reviews 🙌

I addressed all your concerns. As summary,

  • Retry logic now solely rely on the eventing mechanism (no dummy calls)
  • Graceful shutdowns for event listener implemented - proper resource cleanup
  • Justification on thread usage along with proper log levels and comments where necessary

Amazing! I will re-review tomorrow.

Kavindu-Dodan and others added 9 commits August 16, 2023 13:47
@Kavindu-Dodan
improvce cache handling
9a4bc67 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
isolate grpc connector
dd463bb 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
abstrations
ea9395e 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
isolate grpc handling
bf97611 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
reorg
12bdb1a 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
refactor retry logic
b10ef04 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
correct checkstyle
b93d4a8 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
review changes
7be9bed 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan @thisthat
Update providers/flagd/src/main/java/dev/openfeature/contrib/provider…
16fa620 
…s/flagd/grpc/GrpcConnector.java

Co-authored-by: Giovanni Liva <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
continue on interruption
20e3b63 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
improve retry logic
0b9490a 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
avoid wait for connection and fail fast
1ccc28e 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
unit test for retry logic
1af6f4c 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
block initialization
a21363e 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan
add jitter for connection backoff
ca435fc 
Signed-off-by: Kavindu Dodanduwa <[email protected]>
toddbaert
toddbaert approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Manually tested this (though we do have the e2e tests now 😎). LGTM now. Thanks for the improvements.

@Kavindu-Dodan
Copy link
Collaborator Author

Manually tested this (though we do have the e2e tests now 😎). LGTM now. Thanks for the improvements.

Thank you 🙏 I also did several manual validations against a local flagd instance. But yeah, e2e tests to helps

@Kavindu-Dodan Kavindu-Dodan merged commit 2c1060c into open-feature:main Aug 17, 2023
DBlanchard88 pushed a commit to DBlanchard88/java-sdk-contrib that referenced this pull request Apr 29, 2024
@renovate
chore(deps): update actions/checkout digest to f095bcc (open-feature#398
3015571 
)

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

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

@thisthat thisthat Awaiting requested review from thisthat

@liran2000 liran2000 Awaiting requested review from liran2000

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.
5 participants
@Kavindu-Dodan @thisthat @liran2000 @toddbaert @beeme1mr