feat: add upstream strategy `random` #1221

DerRockWolf · 2023-11-04T18:11:35Z

If you want these changes in separate PRs I'll do so 🙂

This PR includes multiple changes:

removal of the resolversPerClient struct used in all upstream group/branch resolvers and replaced by a separate groupName string and a flat upstreamResolverStatus slice.
- The NewXYZResolver() func now accept the new UpstreamGroupConfig struct instead of UpstreamsConfig.
- Doing this allows removal of a lot of redundant code used to select the only existing upstreamResolverStatus slice in the map (Add upstream strategy strict #1093 implemented a "tree" like architecture which resulted in only one slice existing in the map).
some fixed comments and logs
~~removing the context.Canceled check for the parallel_best resolver. I'm not 100% certain but from what I can tell the upstream_resolver Resolve() won't ever return context.Canceled.~~ Edit: we keep the check to not miss it when we will pass a context to upstream_resolver Resolve() func.
And the actual feature: the new random upstream strategy 🎉
- It's basically the same as parallel_best without the race. If the first randomly selected upstream fails a second will be asked.

There are also a few things we could discuss or create issues for:

Passing a context into upstream_resolver Resolve() to allow cancellation of the retry.Do if e.g. a timeout is reached or the other resolver won the race (for parallel_best).
- Currently if a timeout is reached or once one resolver won the race the retry.Do func will still retry any failed calls.
In Add upstream strategy strict #1093 and also in the PR I've used the Config.Upstreams.Timeout to "fail" early and continue with the "try next resolver" logic even though retry.Do might be able to successfully get an answer on one of the two remaining retries.
- We could either get rid of the retry.Do or multiply the timeout for the contexts I've set by 3 to only continue with the next resolvers if all retries failed.
The structs returned by NewXYZResolver all embed the Configurable interface and I'm not 100% sure if this is still needed/useful because the only configuration they currently have is the group name and the Upstreams slice.

Thanks for maintaining blocky ❤️

codecov · 2023-11-04T18:14:21Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (f1a6fb0) 93.70% compared to head (6d335f7) 93.77%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
lists/list_cache.go	60.00%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1221       /-   ##
==========================================
  Coverage   93.70%   93.77%    0.06%     
==========================================
  Files          70       72        2     
  Lines        5687     5892      205     
==========================================
  Hits         5329     5525      196     
- Misses        277      284        7     
- Partials       81       83        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kwitsch · 2023-11-05T00:47:02Z

Looks promising. ♥️

I'll look into it after I fix the parallel processing of unit tests. 🥴

ThinkChaos

I like the idea and most of the changes are great. I wonder if we could make random a specific parallel best configuration: add an option (could just be in the code for now, not necessarily user facing) of how many resolvers to race.
Then random would just set that to 1 :)

config/upstreams.go

DerRockWolf · 2023-11-11T11:23:38Z

I like the idea and most of the changes are great. I wonder if we could make random a specific parallel best configuration: add an option (could just be in the code for now, not necessarily user facing) of how many resolvers to race. Then random would just set that to 1 :)

Good idea, I'll look into it :)

DerRockWolf · 2023-11-12T20:46:18Z

Good idea, I'll look into it :)

I've now implemented the random strategy as custom options for the parallel_best resolver.

I'm not 100% happy with the implementation and it still feels a bit rough.
If you have an idea what could be improved please tell me^^

After my changes the random selection is a little bit different than before (I needed to increase the BeNumerically range from 80-100 to 80-120). I could not found out why, from my point of view the changes shouldn't have impact on the randomness, but maybe you have an idea...

blocky/resolver/parallel_best_resolver_test.go

Lines 292 to 294 in 0e74a3a

    
           // should be 100 ± 20 
        
           // TODO: understand why I needed to adjust this... 
        
           Expect(v).Should(BeNumerically("~", 100, 20))

ThinkChaos · 2023-11-17T14:26:10Z

Sorry for the delay, I missed the original notification. I'll take another look today :)

ThinkChaos

General state is pretty good, looking forward to being able to add a config for how many resolver to query in parallel!

resolver/bootstrap.go

docs/configuration.md

resolver/bootstrap_test.go

resolver/parallel_best_resolver.go

resolver/resolver.go

resolver/parallel_best_resolver.go

Regarding the err check in `resolve()` of `parallel_best_resolver.go` I'm not 100% certain but I don't think the upstream resolver returns err context canceled at all, there are also not tests for it.

resolver/parallel_best_resolver.go

resolver/blocking_resolver.go

- maps.Keys() - docs - parallel_best resolver String() - remove expensive & unnecessary log - return err from retry failed - pre-alloc pickRandom chosenResolvers - fix context comment - upstreamResolverStatus.resolve(): add resolver to err

ThinkChaos · 2023-11-18T20:42:23Z

Did some tiny changes before merging to remove the TODO in the tests and fix a race because of reading and writing Upstreams.Timeout in the tests.
Also about why the numbers changed: for me the using the old numbers for the parallel tests works, so no change!
And for the "random" tests, I think it's not an issue it doesn't exactly match parallel best, as long as we keep it consistant-ish from now on.

Thanks for all the work on this!

DerRockWolf · 2023-11-18T20:53:08Z

Did some tiny changes before merging to remove the TODO in the tests and fix a race because of reading and writing Upstreams.Timeout in the tests. Also about why the numbers changed: for me the using the old numbers for the parallel tests works, so no change! And for the "random" tests, I think it's not an issue it doesn't exactly match parallel best, as long as we keep it consistant-ish from now on.

Thanks for all the work on this!

Thank you for reviewing the PR 😄

DerRockWolf force-pushed the feat/resolver-random branch 2 times, most recently from 20f06ec to ac4b80d Compare November 4, 2023 21:39

ThinkChaos reviewed Nov 5, 2023

View reviewed changes

config/upstreams.go Outdated Show resolved Hide resolved

DerRockWolf force-pushed the feat/resolver-random branch from ac4b80d to 4fe067d Compare November 11, 2023 11:38

DerRockWolf force-pushed the feat/resolver-random branch from 0e74a3a to 3f54720 Compare November 12, 2023 20:52

0xERR0R added the 🔨 enhancement New feature or request label Nov 17, 2023

0xERR0R added this to the v0.23 milestone Nov 17, 2023

ThinkChaos reviewed Nov 17, 2023

View reviewed changes

DerRockWolf added 5 commits November 18, 2023 18:52

some misc changes

b83f156

Regarding the err check in `resolve()` of `parallel_best_resolver.go` I'm not 100% certain but I don't think the upstream resolver returns err context canceled at all, there are also not tests for it.

refactor: getting rid of resolversPerClient and all surrounding logic

827abd9

add random resolver: config

da2b04d

add random resolver tests

176d245

WIP: implement random resolver as custom options in parallel_best

7ce9375

DerRockWolf force-pushed the feat/resolver-random branch from c25d271 to 61d2f1d Compare November 18, 2023 17:52

ThinkChaos reviewed Nov 18, 2023

View reviewed changes

resolver/parallel_best_resolver.go Show resolved Hide resolved

resolver/parallel_best_resolver.go Outdated Show resolved Hide resolved

resolver/parallel_best_resolver.go Outdated Show resolved Hide resolved

ThinkChaos reviewed Nov 18, 2023

View reviewed changes

resolver/blocking_resolver.go Show resolved Hide resolved

code review changes

deee9dc

- maps.Keys() - docs - parallel_best resolver String() - remove expensive & unnecessary log - return err from retry failed - pre-alloc pickRandom chosenResolvers - fix context comment - upstreamResolverStatus.resolve(): add resolver to err

DerRockWolf force-pushed the feat/resolver-random branch from 61d2f1d to deee9dc Compare November 18, 2023 18:29

squash: fix TODO and tweak randomness test comparisons

efd953d

ThinkChaos mentioned this pull request Nov 18, 2023

Improve how we pass config into ParallelBestResolver #1258

Open

ThinkChaos added 2 commits November 18, 2023 15:28

squash: remove debug test focus

8ca2044

squash: fix race in tests

6d335f7

ThinkChaos approved these changes Nov 18, 2023

View reviewed changes

ThinkChaos changed the title ~~resolversPerClient removal & add upstream strategy random~~ feat: add upstream strategy random Nov 18, 2023

ThinkChaos merged commit 94663ee into 0xERR0R:main Nov 18, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add upstream strategy `random` #1221

feat: add upstream strategy `random` #1221

DerRockWolf commented Nov 4, 2023 •

edited

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading

kwitsch commented Nov 5, 2023

ThinkChaos left a comment

DerRockWolf commented Nov 11, 2023

DerRockWolf commented Nov 12, 2023

ThinkChaos commented Nov 17, 2023

ThinkChaos left a comment

ThinkChaos commented Nov 18, 2023

DerRockWolf commented Nov 18, 2023

feat: add upstream strategy random #1221

feat: add upstream strategy random #1221

Conversation

DerRockWolf commented Nov 4, 2023 • edited Loading

codecov bot commented Nov 4, 2023 • edited Loading

Codecov Report

kwitsch commented Nov 5, 2023

ThinkChaos left a comment

Choose a reason for hiding this comment

DerRockWolf commented Nov 11, 2023

DerRockWolf commented Nov 12, 2023

ThinkChaos commented Nov 17, 2023

ThinkChaos left a comment

Choose a reason for hiding this comment

ThinkChaos commented Nov 18, 2023

DerRockWolf commented Nov 18, 2023

feat: add upstream strategy `random` #1221

feat: add upstream strategy `random` #1221

DerRockWolf commented Nov 4, 2023 •

edited

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading