feat: Added hook data#1506
Conversation
|
@mdxabu you will need to force push a commit that is signed off, otherwise the DCO check will not succeed |
Can I squash those commits into one commit, and then I will sign off? |
Yes, you could do that. If you click on the failed DCO action, you schould see a short explanation |
Signed-off-by: mdxabu <abdullahfakrudeen2020@gmail.com>
…d ConcurrentHashMap and use a synchronized HashMap instead Signed-off-by: mdxabu <abdullahfakrudeen2020@gmail.com>
|
@chrfwow, I force push the commits! Now DCO action runs successfully. |
|
The build currently fails because of formatting issues. You can resolve them automatically by running the spotless plugin |
Signed-off-by: mdxabu <abdullahfakrudeen2020@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1506 +/- ##
============================================
+ Coverage 92.82% 93.35% +0.53%
- Complexity 487 492 +5
============================================
Files 46 47 +1
Lines 1170 1189 +19
Branches 103 107 +4
============================================
+ Hits 1086 1110 +24
+ Misses 54 49 -5
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| * Each hook instance gets its own isolated data store that persists only for the duration | ||
| * of a single flag evaluation. | ||
| */ | ||
| public interface HookData { |
There was a problem hiding this comment.
We may be able to use some of the functionality in Structure or it's implementations to do this. We don't need all of those methods, but some wouldn't hurt, I think, and it would reduce duplication and promote consistency.
There was a problem hiding this comment.
@toddbaert I took a closer look and found that Structure enforces that all stored data must be of type Value, which conflicts with the OPENFEATURE SPEC allowing values of any type in hookdata. This restriction makes Structure unsuitable for hook data. As far as I understand it, sticking with @mdxabu 's implementation aligns better with the spec. Pls let me know if I missed anything! 🙏
There was a problem hiding this comment.
Yes, but can use lombok to delegate only some methods.
@aepfli what do you think? You have been working on improving consistency of our POJOs... what is you opinion on this?
There was a problem hiding this comment.
I am actually questioning some of my rewokr based on this findings. -> yes we do have the need for some POJO's to contain only string/numbers/booleans/etc - but i am not sure we need this kind of structure for all of them. especially like the hookcontext is not used for futher evaluation etc. it is used within the same hook, and can have any kind of data. i think that a pure object map would be sufficient in this case. and we should allow all kind of objects (structure limits this, but structure has a dedicated usage for certain operations)
|
@mdxabu how is it going? We do have an internal dependency on this. Can we support somehow? :) |
|
Yeah, Sure! |
Great, let me know how I can support :) |
Co-authored-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Co-authored-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
aepfli
left a comment
aepfli
left a comment
There was a problem hiding this comment.
Looks good from my side. Thank you for the great and valuable addition. I think this will make sharing data between hook steps way easier. Thank you!
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
| // Create hook data for each hook instance | ||
| Map<Hook, HookData> hookDataMap = new HashMap<>(); | ||
| for (Hook hook : reversedHooks) { | ||
| if (hook.supportsFlagValueType(flagValueType)) { | ||
| hookDataMap.put(hook, HookData.create()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need this map and loop? We don't return the map, so it will be GC'ed after the method returns, and we could create the HookData in the loop below like this:
HookContext contextWithHookData = hookCtx.withHookData(HookData.create());
| @Test | ||
| void shouldBeThreadSafe() throws InterruptedException { | ||
| HookData hookData = HookData.create(); | ||
| int threadCount = 10; | ||
| int operationsPerThread = 100; | ||
| CountDownLatch latch = new CountDownLatch(threadCount); | ||
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
|
|
||
| for (int i = 0; i < threadCount; i++) { | ||
| final int threadId = i; | ||
| executor.submit(() -> { | ||
| try { | ||
| for (int j = 0; j < operationsPerThread; j++) { | ||
| String key = "thread-" + threadId + "-key-" + j; | ||
| String value = "thread-" + threadId + "-value-" + j; | ||
| hookData.set(key, value); | ||
| assertEquals(value, hookData.get(key)); | ||
| } | ||
| } finally { | ||
| latch.countDown(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| assertTrue(latch.await(10, TimeUnit.SECONDS)); | ||
| executor.shutdown(); | ||
| } |
There was a problem hiding this comment.
I'd expect this test to fail. HookData is not thread safe anymore. We can remove this test. (In the future, such tests should be done with VmLens, but that is not merged yet)
| return baseContext; | ||
| } | ||
|
|
||
| private static class TestHook implements Hook<String> { |
Co-authored-by: chrfwow <christian.lutnik@dynatrace.com> Signed-off-by: Abu <abdullahfakrudeen2020@gmail.com>
Co-authored-by: chrfwow <christian.lutnik@dynatrace.com> Signed-off-by: Abu <abdullahfakrudeen2020@gmail.com>
|
|
hey @mdxabu - we took a look at your implementation and it is a good foundation, but we have seen some issues, with how data was enriched, and also with the object churn (creating a lot of objects). So we gave it a shot on our own here #1587 - we used your pr as a basis, and we will keep all of your commits. we just wanted to make sure you are okay with this. Thank you for the effort and the foundation. |
|
Yeah, I'm okay with that. I'm glad you're asking for my opinion! I like this community! |
6 participants
This PR
Added the Hook Data
Related Issues
Fixes #1472
@beeme1mr, Please review this if there is any problem or if I did anything wrong, ping me!