Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(flags): dynamic cohort matching in rust #25776
feat(flags): dynamic cohort matching in rust #25776
Changes from 33 commits
ca431b5
e89f169
4f20e07
ed00224
fb8aab8
899a99c
896c31a
eeea8cc
d02baec
39dad2d
db8cd8d
43cda76
8d2ab85
9ccf479
797adbe
71def67
27af814
4c49bc4
d4af2f0
870f719
57d9885
9eb0f18
3cfc590
3e8e5d2
3528b31
77059f3
09317c4
43e8692
3a65683
a5812e6
fd52b24
59f7c10
8066aff
4d5ecd9
4012ebe
8ededb1
fe37b04
bc38940
41d3db3
0a409f4
0dd1c0b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caching lib with support for TTL and feature weighting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a heads up, this is already in the workspace, you can probably pull it in (we're using it in error tracking).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, same as below re: postgres_reader I suppose, but I know from the type that the cache is per-team (it's got TeamId as a key), and I know it's caching cohorts. You can be shorter here, the type shows up everywhere it's used.
This is purely taste though, if you disagree feel free to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but if the variable name is the same as the type name, I go for stuff like "pr: PostgresReader" - the ide tells me everything I need to know about it anyway. I'd make it
reader: PostgresReader
in the struct declarationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought about casts generally, I think this is totally fine and you shouldn't pay the CI time to change it:
I'd almost argue for a raw
unwrap
here (or anexpect
with a helpful message), under the consideration you probably do want to fail loudly if a team has more thanu32::MAX
cohorts, but also, you'll never end up in this situation because fetching them would bring down postgres, you'd OOM, etc, so I'd then almost go for anas
cast instead, knowing the truncation will never happen.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default strikes me as quite low, I'd bump it an order of magnitude (or set it an order of magnitude larger) - that's a pure gut feeling though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, I know it's
for_team
because you ask me for a team id (this is the last I'll leave of these, mostly just highlighting you can often be more concise without losing information the caller needs here, but again, taste)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a note: I'm a fan of taking a manager-wide lock here (called, say,
fetch_lock: Mutex<()>
in the struct decl), to prevent multiple fetches to the same cohorts, but that's totally an optimisation you can skip until it becomes a problem (basically until postgres gives out).