Skip to content
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

fix: rpc caching not behaving as expected (cleared too often) #1115

Merged
merged 15 commits into from
Jan 20, 2025

Conversation

aepfli
Copy link
Member

@aepfli aepfli commented Dec 28, 2024

With this pull request, I am fixing a regression within the RPC provider. We cleared the cache when there was an error, but we had to wait until we hit the error state and not stay in the stale state for the clean to happen. I also used this to normalize the eventing a little bit and reduce the complexity.

Additionally, I updated the e2e tests to the newest Gherkin version:

  • missing tests: targetUri (will be own task)
  • error and mismatch as the spec file does not match this configuration anymore (will be added to the gherkin files)

@aepfli aepfli force-pushed the feat/gherkin-rework branch 7 times, most recently from d2b3cd4 to 910031e Compare December 28, 2024 18:15
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from 4489649 to d61effe Compare January 3, 2025 19:32
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from b349cc3 to 0a3b414 Compare January 13, 2025 12:01
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from 5822abc to 5678da0 Compare January 15, 2025 10:54
@aepfli aepfli changed the title test(flagd): rework e2e tests to new format feat(flagd): fix flagd rpc caching not behaving as expected (cleared too often) Jan 15, 2025
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from 6afae99 to 838b248 Compare January 15, 2025 11:07
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.

Amazing work. Minor nit and I made one small change here which I hope you don't mind. Otherwise on my system the timeout was not detected.

@toddbaert toddbaert changed the title feat(flagd): fix flagd rpc caching not behaving as expected (cleared too often) fix: rpc caching not behaving as expected (cleared too often) Jan 15, 2025
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from 2a4cd90 to 07052a0 Compare January 15, 2025 20:44
@aepfli
Copy link
Member Author

aepfli commented Jan 15, 2025

All dependencies are resolved. Ready to merge ;)

@aepfli aepfli force-pushed the feat/gherkin-rework branch from 07052a0 to ff5bccf Compare January 16, 2025 10:17
…s/flagd/resolver/process/storage/connector/grpc/GrpcStreamConnector.java

Co-authored-by: warber <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from 8e2aeae to 70ee2fa Compare January 16, 2025 17:37
@aepfli aepfli force-pushed the feat/gherkin-rework branch from 70ee2fa to 338f367 Compare January 16, 2025 17:43
@toddbaert toddbaert requested review from chrfwow and warber January 16, 2025 18:15
@aepfli aepfli force-pushed the feat/gherkin-rework branch 2 times, most recently from 30e5f82 to a67b36c Compare January 16, 2025 19:33
@aepfli aepfli force-pushed the feat/gherkin-rework branch from a67b36c to a14931f Compare January 17, 2025 07:58
@toddbaert
Copy link
Member

toddbaert commented Jan 20, 2025

I pushed one more small change to move all the volatile fields on the provider into a dedicated object to be used as the intrinsic lock in our synchronized blocks. I think this has the benefit of being very clear but also it unifies all our locking onto a single object.

@toddbaert toddbaert merged commit b4fe2f4 into open-feature:main Jan 20, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants