Skip to content

Fix PercentageFilter inequality bound - #156

Merged
Jimmy Campbell (jimmyca15) merged 1 commit into
microsoft:mainfrom
neiljohari:bug-percentage-filter-edge-case
Dec 3, 2021
Merged

Jimmy Campbell (jimmyca15) merged 1 commit into
microsoft:mainfrom
neiljohari:bug-percentage-filter-edge-case

Conversation

@neiljohari

@neiljohari Neil Johari (neiljohari) commented Dec 3, 2021 •

Copy link
Copy Markdown
Contributor

NextDouble() ∈ [0, 1). We use a strict inequality, so a 0% rollout value sporadically returns true as 0⩽0.

The fix is to use a non-strict inequality, <. The upper bound still works: 0.999... < 1.

We noticed this internally because feature flags were occasionally tripping even with 0% rollout. This appears to be the root cause.

NextDouble() ∈ [0, 1). We use a strict inequality, so a 0%
value sporadically returns true as 0⩽0.

The fix is to use a non-strict inequality, <. The upper bound still works: 0.999... < 1.
@neiljohari

Copy link
Copy Markdown
Contributor Author

Additionally, the CLA GHA didn't appear to trip. This PR is provided as part of my employment with https://github.com/roblox, I have the needed authorization to sign the CLA so please do send me a link to an agreement I can sign!

@jimmyca15

Copy link
Copy Markdown
Member

Neil Johari (@neiljohari) The license check mentions that all CLA requirements are met. Have you already contributed to other Microsoft repos?

@neiljohari

Copy link
Copy Markdown
Contributor Author

Jimmy Campbell (@jimmyca15) Thanks for the super speedy response! I don't believe I have previously contributed, I think this is my first one

@jimmyca15
Jimmy Campbell (jimmyca15) merged commit e3eb4da into microsoft:main Dec 3, 2021
@jimmyca15

Copy link
Copy Markdown
Member

I took a look. The license check did run. For small enough changes it doesn't prompt CLA sign.

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.

2 participants