Support for resolving sticky assignments #31

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.

Sign up for GitHub

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

Jump to bottom

Merged

vahidlazio merged 18 commits into main from update-resolve-proto

Sep 30, 2025

+727 −61

Contributor

vahidlazio commented Sep 18, 2025

No description provided.

vahidlazio added 4 commits

September 18, 2025 14:59


          add basic support for sticky assignments

f118eeb


          more sticky

d9c9213


          fixup! more sticky

60cbc3e


          cleanup

329ab21

vahidlazio force-pushed the update-resolve-proto branch from 867d430 to be6d47a Compare

September 18, 2025 15:36


          fixup! cleanup

1a818bc

vahidlazio force-pushed the update-resolve-proto branch from be6d47a to 1a818bc Compare

September 18, 2025 15:48

vahidlazio added 2 commits

September 19, 2025 11:35


          add resolving sticky

b49a7e0


          fail fast on the first missing materialization info

vahidlazio force-pushed the update-resolve-proto branch from 474d660 to 9998135 Compare

September 19, 2025 09:46

vahidlazio marked this pull request as ready for review

September 19, 2025 09:48


          add documentation about how sticky assignments are resolved and what …

…is the algorithm to discover missing materializations and resolve

vahidlazio force-pushed the update-resolve-proto branch from b8ccb02 to 6073069 Compare

September 19, 2025 10:05

vahidlazio changed the title ~~Update resolve proto~~ Support for resolving sticky assignments

andreas-karlsson reviewed

View reviewed changes

Contributor

andreas-karlsson left a comment

This implementation is more complex than I would like. It occurred to me. The so called discovery mode, where we find out what materializations are necessary, isn't it wasteful to do this on the resolve path? We should be able to do this once whenever we get a new state?

I also think this could be a lot simplified after we switch to resolving flags one by one?

I also wonder about the optimisation. Will we let customers decide or should we decide which sticky mode to use?

confidence-resolver/protos/confidence/flags/resolver/v1/api.proto Outdated Show resolved Hide resolved

confidence-resolver/protos/confidence/flags/resolver/v1/api.proto Outdated Show resolved Hide resolved

confidence-resolver/protos/confidence/flags/resolver/v1/api.proto Outdated Show resolved Hide resolved


          wrap the resolve request in resolve with sticky req

242ac57

vahidlazio requested a review from andreas-karlsson

September 23, 2025 13:53


          fixup! wrap the resolve request in resolve with sticky req

b1bd52f

vahidlazio force-pushed the update-resolve-proto branch from 9eb963f to b1bd52f Compare

September 23, 2025 13:59

andreas-karlsson reviewed

View reviewed changes

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

vahidlazio added 4 commits

September 24, 2025 12:08


          fix clippy

d5dde2f


          fixup! fix clippy

577812b


          fix clippy

e9da866


          move to wasm_api

43cc4e0

vahidlazio force-pushed the update-resolve-proto branch from ab5c7f1 to 43cc4e0 Compare

September 24, 2025 11:07

andreas-karlsson reviewed

View reviewed changes

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/protos/confidence/flags/resolver/v1/wasm_api.proto Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

vahidlazio added 2 commits

September 25, 2025 11:12


          move collect deps out of resolve flag

8ee81ff


          fixup! move collect deps out of resolve flag

691e0ff

andreas-karlsson reviewed

View reviewed changes

Contributor

andreas-karlsson left a comment

Some tests?

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs

Comment on lines +658 to +660

    
                                  ResolveResult::MissingMaterializations(_) => {

                                      Err("sticky assignments is not supported".to_string())

                                  }

Contributor

andreas-karlsson Sep 25, 2025

Is this what we want? I.e. you can't ignore sticky? I thought we could. But if not I guess there is no need for a separate resolve_with_sticky fn?

Contributor Author

vahidlazio Sep 26, 2025

in this case we fallback to the default value.

confidence-resolver/src/lib.rs Show resolved Hide resolved

confidence-resolver/src/lib.rs Outdated Show resolved Hide resolved


          fix comments

fd11855

vahidlazio force-pushed the update-resolve-proto branch from a02aa71 to fd11855 Compare

September 26, 2025 11:34

andreas-karlsson approved these changes

View reviewed changes

Contributor

andreas-karlsson left a comment

Left three comments, but none of them are really important, so approved.

confidence-resolver/protos/confidence/flags/resolver/v1/wasm_api.proto Outdated Show resolved Hide resolved

confidence-resolver/protos/confidence/flags/resolver/v1/wasm_api.proto Outdated Show resolved Hide resolved

confidence-resolver/src/lib.rs Show resolved Hide resolved


          move the materialization types to the response

9e3276e

vahidlazio merged commit a180d2e into main

6 checks passed

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

Labels

None yet