-
Notifications
You must be signed in to change notification settings - Fork 51
feat: json logic operators for flagd in-process provider #434
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: json logic operators for flagd in-process provider #434
Conversation
...src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/model/FlagParser.java
Show resolved
Hide resolved
...main/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/Fractional.java
Show resolved
Hide resolved
.../java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java
Show resolved
Hide resolved
Unless the difference is highly nuanced and unlikely to be seen in normal usage, I think this is a deal-breaker. We need the evaluators to resolve identically, or it could result in weird behavior if applications have different components written in different languages. If we need to change something about the algorithm (convert numbers to UTF-8 strings in a consistent way, for example) that's fine - we can make that update. Can you provide details as to exactly what is inconsistent? For example, could I make the gherkin suite pass? |
FYI I see the same failure locally as what appears to be happening in the CI: https://github.com/open-feature/java-sdk-contrib/actions/runs/6165511503/job/16733420790?pr=434#step:6:3109 |
this is resolved locally. I will push the changes with review changes :) |
This looks good to me, pending a better understanding of this point and the additional test I mentioned. If the differences in the fractional evaluation are slim or can easily be avoided or fixed later, I can approve. |
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
81e68f9
to
bc3a4eb
Compare
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Sorry for the late response. I did some tests with Java and Go but I was unable to get same hash generated from both. To start with, flagd use twmb/murmur3 and it uses unsigned integers 1. I copied our usage from apache hive 2, and this uses Java long, which are signed values. Given that go uses UTF-8 by default and I mandate encoding in Java to UTF-8, this shouldn't be the root cause. However, I assume there's something with signed vs unsigned number usage (unfortunately I am not an expert on hashing ;) ) I will try with Guava 3 and see if we get same results in Java. Footnotes
|
I could be misunderstanding your point, but the number representations the implementation of the hash algo uses should be irrelevant. ie: SHA1 implemented in any language with any number representation should be deterministic for any input. My suspicion is that our input bytes are actually not the same in both implementations. |
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Finally found the reason and the fix for this mismatch. flagd originally used 64-bit version of the murmur3 algorithm. This seems to not come from the original algorithm. I have swithched the algorithm to the 32-bit version 1 and this matches perfectly with the Java version as well. I have updated this PR with the change -2386d16 Footnotes |
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Great job. That makes sense. The murmur3 spec doesn't even include a 64bit version, and the 128bit is unstable across architectures. 32 is the right choice! |
Signed-off-by: Kavindu Dodanduwa <[email protected]>
99839d0
to
f3354a9
Compare
…gin to v3.21.0 (open-feature#434) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This PR
Fixes #411
Introduces and binds the following operators,
Note that, implementations use following libraries
from Apache Hive extracted with license intactfrom apache commonNote that,Fractional
evaluations are not 100% compatible with flagd implementation from GO. This is because of the usage of signed long values and related overflowing happening in respective implementations of Java.Fractional evaluations are cross-language compatible thanks to murmur3 32 bit usage across languages.