feat: v2 evaluate boolean #1806
Conversation
Codecov Report
@@ Coverage Diff @@
## eval-v2 #1806 +/- ##
===========================================
- Coverage 70.32% 69.30% -1.03%
===========================================
Files 58 63 +5
Lines 5570 5766 +196
===========================================
+ Hits 3917 3996 +79
- Misses 1426 1543 +117
Partials 227 227
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| JOIN constraints c ON (rs.segment_key = c.segment_key) | ||
| ) rss ON (r.id = rss.rollout_id) | ||
| WHERE r.flag_key = $1 AND r.namespace_key = $2 | ||
| ORDER BY r."rank" ASC |
There was a problem hiding this comment.
this is a monster query, im wondering if it would be simpler and potentially faster because all of the joins to do:
- get all rollouts for flag from
rolloutstable by flagKey, sorted byrank, this will give you a slice of[]rolloutsthat are naturally ordered - create a
map[rolloutID] => rolloutas an index for each type (percentage & variant) - query for all
percentage_rolloutsby ID, using anIN()query with their IDs from step 2 - query for all
segment_rolloutsby ID, using anIN()query with their IDs from step 2 - now sort the final results looping through the id's in order from step 1, and built up the final result set using the actual
percentage_rolloutorsegment_rollout.
I guess what I'm saying is I wonder which approach will be faster, implementing it all in SQL or in Go?
Perhaps we should create some benchmarks and try both approaches? We could create another issue for this to do before it goes 'live'. WDYT?
There was a problem hiding this comment.
@markphelps Benchmarks sound good. I meant to call this query out initially, I should have followed up with that.
There was a problem hiding this comment.
To further this. I think we should break this PR in two like we originally planned.
Focus on delivering the server definition first. Just leave the storage requirement as an interface with no implementation.
Then implement the storage as a follow up PR as there is a lot to digest here.
FLI-459 is the server issue
FLI-454 is the storage implementation
There was a problem hiding this comment.
@GeorgeMac Agreed. I'll move that to another PR.
yquansah
force-pushed
the
v2-evaluate-boolean
branch
from
29ca955
to
7c03b2e
Compare
|
|
||
| for _, rollout := range rollouts { | ||
| if rollout.Rank < lastRank { | ||
| return &rpcEvaluation.BooleanEvaluationResponse{}, errs.ErrInvalidf("rollout rank: %d detected out of order", rollout.Rank) |
There was a problem hiding this comment.
💅 should be able to start returning nil again if we stop setting reason when error is non-nil.
Also, I think this should be and internal error as we control the rank field.
| return &rpcEvaluation.BooleanEvaluationResponse{}, errs.ErrInvalidf("rollout rank: %d detected out of order", rollout.Rank) | |
| return nil, fmt.Errorf("rollout rank: %d detected out of order", rollout.Rank) |
| @@ -12,26 +12,20 @@ import ( | |||
| "go.flipt.io/flipt/internal/server/metrics" | |||
| "go.flipt.io/flipt/internal/storage" | |||
| "go.flipt.io/flipt/rpc/flipt" | |||
| rpcEvaluation "go.flipt.io/flipt/rpc/flipt/evaluation" | |||
There was a problem hiding this comment.
💅 tend to just lowercase package names:
| rpcEvaluation "go.flipt.io/flipt/rpc/flipt/evaluation" | |
| rpcevaluation "go.flipt.io/flipt/rpc/flipt/evaluation" |
There was a problem hiding this comment.
Definitely. Let me get that fixed up.
|
looks great! noticed a few areas of missing coverage in the evaluation server code (at least according to CodeCov 😐 ) if the report is accurate, would it be possible to get these covered?:
|
Boolean evaluation logic for the v2 evaluate endpoints. This PR also addresses some comments from the upstream branch, as far as styling and renaming.
Completes FLI-459