-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: context propagation #848
feat: context propagation #848
Conversation
Wow thanks @ssharaev ! We'll get this reviewed ASAP, but it's KubeCon this week so many maintainers are traveling or will be soon, so patience is appreciated! I'm adding @beeme1mr and @lukas-reining even though they aren't Java maintainers, because they both worked on the context propagation spec. |
@toddbaert , thank you! |
src/main/java/dev/openfeature/sdk/NoOpTransactionContextPropagator.java
Outdated
Show resolved
Hide resolved
cdf2f6f
to
d8c39f8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #848 +/- ##
============================================
+ Coverage 95.30% 95.64% +0.34%
- Complexity 370 384 +14
============================================
Files 34 36 +2
Lines 852 873 +21
Branches 52 52
============================================
+ Hits 812 835 +23
+ Misses 21 20 -1
+ Partials 19 18 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@ssharaev great work 👏
I think this is the right choice. But we should document this fact as well as show how to register
Yes, this is the wrong setup of the test. Thank you for seeing this and fixing it. I left some review comments, please check them. Once done, I can approve this :) |
5394cf4
to
7b96c2f
Compare
@ssharaev could you please check the check style failure [1] You can execute them locally with |
7b96c2f
to
0af8a3e
Compare
Yes, of course! |
0af8a3e
to
cf87999
Compare
LGTM 🙌 Thank you again @ssharaev |
2629d21
to
c888563
Compare
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.
This looks great to me, thanks @ssharaev!
The only thing I would like to change is the test that verifies that the values are actually thread independent.
contextPropagator.setTransactionContext(firstContext); | ||
EvaluationContext firstThreadContext = contextPropagator.getTransactionContext(); | ||
assertSame(firstContext, firstThreadContext); | ||
|
||
FutureTask<EvaluationContext> futureTask = new FutureTask<>(callable); | ||
Thread thread = new Thread(futureTask); | ||
thread.start(); | ||
EvaluationContext secondThreadContext = futureTask.get(); | ||
|
||
assertSame(secondContext, secondThreadContext); | ||
assertSame(firstContext, firstThreadContext); |
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.
Here we should also get and assert the value of the firstThreadContext
after line 52.
Currently as far as I can tell the test does not verify that firstThreadContext
is not changed by the callable in the second thread, as the value of firstThreadContext
is read before (line 46) the second thread is started (line 51)
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.
Agreed.
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.
Yes, you're right, line 55 checked nothing. Thank you for finding this!
Replaced it with assertSame(firstContext, contextPropagator.getTransactionContext());
@ssharaev I'll give this a thorough review tomorrow. |
c888563
to
8c5ee7e
Compare
* @param invocationContext invocation context | ||
* @return merged evaluation context | ||
*/ | ||
private EvaluationContext mergeEvaluationContext( |
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.
Nice idea to extract this.
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.
thanks! :)
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 spent time looking through this, and besides the minor point @lukas-reining brought up, it looks very good. Very nice to have this done, thanks so much!
Would you like an invite to the OpenFeature org @ssharaev ? There's no obligation, but it allows us to ping you, etc.
8c5ee7e
to
a7f2af9
Compare
@toddbaert , thank you for your review!
Wow, I would be happy to join OpenFeature org, I really love, what you're doing! |
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.
Looks good to me, thank you @ssharaev :)
Signed-off-by: Sviatoslav Sharaev <sviatoslav.sharaev@gmail.com>
Signed-off-by: Sviatoslav Sharaev <sviatoslav.sharaev@gmail.com>
Co-authored-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com> Signed-off-by: Sviatoslav Sharaev <sviatoslav.sharaev@gmail.com>
Signed-off-by: Sviatoslav Sharaev <sviatoslav.sharaev@gmail.com>
a7f2af9
to
c33f668
Compare
Quality Gate passedIssues Measures |
This PR
Related Issues
Fixes #819
Notes
OpenFeatureAPI
should useNoOpTransactionContextPropagator
as a defaultTransactionContextPropagator
, or if it can simply benull
.FlagEvaluationSpecTest.multi_layer_context_merges_correctly()
- attributes were put in the sameHashMap
twice. Have a look at it, please, and if I'm mistaken, I'll revert the changes.