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

chore: replace xxh3 with murmur3 in bucket algorithm #846

Merged
merged 5 commits into from
Aug 21, 2023
Merged

chore: replace xxh3 with murmur3 in bucket algorithm #846

merged 5 commits into from
Aug 21, 2023

Conversation

craigpastro
Copy link
Member

This PR

Replace xxh3 with murmur3 in fractional evaluation's bucketing algorithm.

Related Issues

Part 1 of #843.

Notes

Follow-up Tasks

How to test

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>
@craigpastro craigpastro requested a review from a team as a code owner August 17, 2023 02:51
@netlify
Copy link

netlify bot commented Aug 17, 2023

Deploy Preview for polite-licorice-3db33c canceled.

Name Link
🔨 Latest commit 99e4ffb
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/64e36f8863c1300008e7d7bb

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.

Thanks!

@toddbaert toddbaert requested review from a team August 17, 2023 16:19
@beeme1mr
Copy link
Member

Hey @craigpastro, thanks for working on this! Could you please update the technical section in the docs to reference to murmur3 instead of xxh3?

https://github.com/open-feature/flagd/blob/main/docs/configuration/fractional_evaluation.md#fractional-evaluation-technical-description

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>
@craigpastro
Copy link
Member Author

craigpastro commented Aug 17, 2023

👋 @beeme1mr. Thank you. I've updated the docs and linked to the reference implementation. I don't know if that is the right thing to link to, but I didn't find anything better.

Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good. Seems the test is now evenly spreadout (contain all four colours)

@craigpastro yes you have updated the docs per current main branch status. However, this will conflict with change proposed through PR #833

@agardnerIT fyi

@craigpastro
Copy link
Member Author

👋 @Kavindu-Dodan. Thank you for the review and for noticing that the documentation will conflict. I can rebase if #833 is merged first.

Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 I left a minor comment for you to consider @craigpastro

toddbaert and others added 2 commits August 21, 2023 08:59
Co-authored-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert changed the title refactor: replace xxh3 with murmur3 chore: replace xxh3 with murmur3 in bucket algorithm Aug 21, 2023
@toddbaert
Copy link
Member

toddbaert commented Aug 21, 2023

I think this is causing the lint failure.

@toddbaert toddbaert merged commit c3c9e4e into open-feature:main Aug 21, 2023
13 checks passed
@github-actions github-actions bot mentioned this pull request Aug 21, 2023
@craigpastro craigpastro deleted the fractional-evaluator-improvments branch August 21, 2023 16:05
beeme1mr pushed a commit that referenced this pull request Aug 31, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>flagd: 0.6.4</summary>

##
[0.6.4](flagd/v0.6.3...flagd/v0.6.4)
(2023-08-30)


### 🐛 Bug Fixes

* **deps:** update module github.com/cucumber/godog to v0.13.0
([#855](#855))
([5b42486](5b42486))
* **deps:** update module github.com/open-feature/flagd/core to v0.6.3
([#794](#794))
([9671964](9671964))


### 🧹 Chore

* **deps:** update golang docker tag to v1.21
([#822](#822))
([effe29d](effe29d))
</details>

<details><summary>flagd-proxy: 0.2.9</summary>

##
[0.2.9](flagd-proxy/v0.2.8...flagd-proxy/v0.2.9)
(2023-08-30)


### 🐛 Bug Fixes

* **deps:** update module github.com/open-feature/flagd/core to v0.6.3
([#794](#794))
([9671964](9671964))


### 🧹 Chore

* **deps:** update golang docker tag to v1.21
([#822](#822))
([effe29d](effe29d))
</details>

<details><summary>core: 0.6.4</summary>

##
[0.6.4](core/v0.6.3...core/v0.6.4)
(2023-08-30)


### 🐛 Bug Fixes

* **deps:** update kubernetes packages to v0.28.0
([#841](#841))
([cc195e1](cc195e1))
* **deps:** update kubernetes packages to v0.28.1
([#860](#860))
([f3237c2](f3237c2))
* **deps:** update module github.com/open-feature/open-feature-operator
to v0.2.36 ([#799](#799))
([fa4da4b](fa4da4b))
* **deps:** update module golang.org/x/crypto to v0.12.0
([#797](#797))
([edae3fd](edae3fd))
* **deps:** update module golang.org/x/net to v0.14.0
([#798](#798))
([92c2f26](92c2f26))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.15.1
([#795](#795))
([13d62fd](13d62fd))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.16.0
([#856](#856))
([88d832a](88d832a))


### ✨ New Features

* add flag key to hash in fractional evaluation
([#847](#847))
([ca6a35f](ca6a35f))
* add gRPC healthchecks
([#863](#863))
([da30b7b](da30b7b))
* support nested props in fractional evaluator
([#869](#869))
([50ff739](50ff739))


### 🧹 Chore

* deprecate fractionalEvaluation for fractional
([#873](#873))
([243fef9](243fef9))
* replace xxh3 with murmur3 in bucket algorithm
([#846](#846))
([c3c9e4e](c3c9e4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[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.

5 participants