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

feat: json logic operators for flagd in-process provider #434

Merged
merged 13 commits into from
Sep 14, 2023

Conversation

Kavindu-Dodan
Copy link
Collaborator

@Kavindu-Dodan Kavindu-Dodan commented Sep 12, 2023

This PR

Fixes #411

Introduces and binds the following operators,

  • Fractional
  • SemVer
  • StringComp

Note that, implementations use following libraries

  • semver4j : for semantic version parsing and validations
  • Murmur3 : from Apache Hive extracted with license intact from apache common

Note 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.

@toddbaert
Copy link
Member

toddbaert commented Sep 13, 2023

Note 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.

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?

@toddbaert
Copy link
Member

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

@Kavindu-Dodan
Copy link
Collaborator Author

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 :)

@toddbaert toddbaert self-requested a review September 13, 2023 19:57
@toddbaert
Copy link
Member

toddbaert commented Sep 13, 2023

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 <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Collaborator Author

Note 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.

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?

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

  1. https://github.com/twmb/murmur3/blob/master/murmur64.go#L45-L48

  2. https://github.com/apache/hive/blob/master/storage-api/src/test/org/apache/hive/common/util/TestMurmur3.java

  3. https://github.com/google/guava/blob/master/guava/src/com/google/common/hash/Murmur3_32HashFunction.java

@toddbaert
Copy link
Member

Note 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.

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?

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

  1. https://github.com/twmb/murmur3/blob/master/murmur64.go#L45-L48
  2. https://github.com/apache/hive/blob/master/storage-api/src/test/org/apache/hive/common/util/TestMurmur3.java
  3. https://github.com/google/guava/blob/master/guava/src/com/google/common/hash/Murmur3_32HashFunction.java

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 <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Collaborator Author

Note 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.

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?

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

  1. https://github.com/twmb/murmur3/blob/master/murmur64.go#L45-L48
  2. https://github.com/apache/hive/blob/master/storage-api/src/test/org/apache/hive/common/util/TestMurmur3.java
  3. https://github.com/google/guava/blob/master/guava/src/com/google/common/hash/Murmur3_32HashFunction.java

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.

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

  1. https://github.com/open-feature/flagd/pull/913

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@toddbaert
Copy link
Member

Note 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.

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?

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

  1. https://github.com/twmb/murmur3/blob/master/murmur64.go#L45-L48
  2. https://github.com/apache/hive/blob/master/storage-api/src/test/org/apache/hive/common/util/TestMurmur3.java
  3. https://github.com/google/guava/blob/master/guava/src/com/google/common/hash/Murmur3_32HashFunction.java

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.

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

  1. fix: use 32bit murmur calculation (64 is not stable) flagd#913

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 <kavindudodanduwa@gmail.com>
@Kavindu-Dodan Kavindu-Dodan merged commit 485c8a3 into open-feature:main Sep 14, 2023
4 checks passed
DBlanchard88 pushed a commit to DBlanchard88/java-sdk-contrib that referenced this pull request Apr 29, 2024
…gin to v3.21.0 (open-feature#434)

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flagd] Intial version of in-process provider
3 participants