authorize: build evaluators cache in parallel #4722
Conversation
| evals, errs := errgrouputil.Build(ctx, builders...) | ||
| if len(errs) > 0 { | ||
| for _, err := range errs { | ||
| log.Error(ctx).Msg(err.Error()) |
There was a problem hiding this comment.
If a single error will fail this operation, I wonder if we should take advantage of the error handling behavior built in to errgroup.Group, and have Build() return the first error encountered?
There was a problem hiding this comment.
that would be behaviour in authorize, but not in databroker package, which is why we return results and errors separately so that calling package may decide what to do next. use of errgroup is primarily for the SetLimit convenience
| // and returns the results and any errors. | ||
| func Build[T any]( | ||
| ctx context.Context, | ||
| builders ...BuilderFunc[T], |
There was a problem hiding this comment.
I'm not entirely sure about this API. I'm wondering if there's an alternative where we wouldn't need to allocate this entire slice of functions up front.
Have you considered something more like this?
func Build[I, O any](ctx context.Context, f func(I) (O, error))There was a problem hiding this comment.
agree its not the most elegant interface. i primarily wanted to concentrate the logic of concurrent building (and the magic formula of SetLimit(runtime.NumCPU()/2+1)) somewhere in one place for the config building speedup in v0.24
we can certainly iterate on a design that looks cleaner, as we still seem to have a few places where we can build heavy configs in parallel.
|
|
||
| fn := func(i int) func() error { | ||
| return func() error { | ||
| result, err := builders[i](ctx) |
There was a problem hiding this comment.
I'm not sure about this context propagation either. Would it be better to check for cancellation here (or around line 41?), and return early if cancelled, without the builder function needing to know about the errgroup context?
* authorize: build evaluators cache in parallel * session: add unit tests for gRPC wrapper methods (#4713) * core/config: add support for maps in environments (#4717) * reconciler: allow custom comparison function (#4726) * add loopvar alias --------- Co-authored-by: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com> Co-authored-by: Caleb Doxsey <cdoxsey@pomerium.com>
authorize: build evaluators cache in parallel (#4722) * authorize: build evaluators cache in parallel * session: add unit tests for gRPC wrapper methods (#4713) * core/config: add support for maps in environments (#4717) * reconciler: allow custom comparison function (#4726) * add loopvar alias --------- Co-authored-by: Denis Mishin <dmishin@pomerium.com> Co-authored-by: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com> Co-authored-by: Caleb Doxsey <cdoxsey@pomerium.com>
Summary
Builds Rego evaluators in parallel, which makes sense for large route counts.
Also moves the parallel building code into separate utility package.
Related issues
User Explanation
Checklist
improvement/bug/ etc)