Skip to content

change(rc): Update Remote Config condition evaluation hashing - #2760

Merged
kjelko merged 10 commits into
masterfrom
ssrc-hash
Nov 7, 2024
Merged

change(rc): Update Remote Config condition evaluation hashing#2760
kjelko merged 10 commits into
masterfrom
ssrc-hash

Conversation

@kjelko

@kjelkokjelko commented Nov 5, 2024

Copy link
Copy Markdown
Contributor

Update remote config condition evaluation hashing

@lahirumarambalahirumaramba changed the title Update remote config condition evaluation hashingchange(rc): Update Remote Config condition evaluation hashingNov 5, 2024
@lahirumarambalahirumaramba added release-note release:stage Stage a release candidate labels Nov 5, 2024

@lahirumarambalahirumaramba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thank you!

Comment threadsrc/remote-config/condition-evaluator-internal.ts Outdated
}

return hash64;
const hex = createHash('sha256').update(seededRandomizationId).digest('hex');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking (no action req'd): we had a 64 bits hash before, so we don't need 256 bits, but sha256 is broadly available and there's no noticeable performance difference, so sha256 is fine.


// Manually negate the hash if its value is less than 0, since Math.abs doesn't
// support BigInt.
if (hash64 < 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking (no action req'd): we're now directly instantiating the bigint with the hash, rather than coercing it to behave like a 64bit signed Java long, so it will always be positive.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:stageStage a release candidaterelease-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kjelko@erikeldridge@lahirumaramba