-
Notifications
You must be signed in to change notification settings - Fork 225
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
Support context propagation #925
base: master
Are you sure you want to change the base?
Conversation
- context.Context instance passed in ContextEval can be propagated to binding function to cancel the process.
I've confirmed that all tests pass. Please review this 🙏 . |
/gcbrun |
@goccy I'll give it a look. If there are changes in the top-level APIs, I'll probably ask for parallel implementations for things like |
@goccy looks like there were some failures in the sub-modules:
|
@TristonianJones Thank you for your quickly response. I've fixed that build error and I confirmed passing all tests by |
@TristonianJones type Interpretable {
Eval(activation Activation) ref.Val
+ EvalContext(ctx context.Context, activation Activation) ref.Val
}
type Qualifier {
Qualify(vars Activation, obj any) (any, error)
QualifyIfPresent(vars Activation, obj any, presenceOnly bool) (any, bool, error)
+ QualifyContext(ctx context.Context, vars Activation, obj any) (any, error)
+ QualifyIfPresentContext(ctx context.Context, vars Activation, obj any, presenceOnly bool) (any, bool, error)
} Also, should I add the fields for context interface to the |
@goccy Sorry for the delayed reply. I've been weighing this approach with a previous one we refer to as iterative-eval. The idea behind iterative eval is that context dependent functions are initially stubbed with synchronous versions which capture arguments and return The benefit of such an approach is that it's async in terms of a synchronous process and that it allows for batching of async calls. The drawback is that it's somewhat complex and doesn't completely fit with user expectations regarding which functions are executed when. Part of the reason we haven't adopted either approach is because of the plumbing changes required to support them. Effectively, you need parallel methods to the ones that exist today. Since I'm not sure who might be using the I'll need to think about this more. Thanks for your patience! |
@TristonianJones Thank you for your explanation ! To avoid breaking changes, what about the following approach of defining a new interface and using it only internally? If this looks good, I will try to implement it. type InterpretableContext interface {
Interpretable
EvalContext(ctx context.Context, activation Activation) ref.Val
}
type QualifierContext interface {
Qualifier
QualifyContext(ctx context.Context, vars Activation, obj any) (any, error)
QualifyIfPresentContext(ctx context.Context, vars Activation, obj any, presenceOnly bool) (any, bool, error)
} |
I apologize for the additional mention, but I've implemented this without any breaking changes. I would appreciate it if you could review it when you have time. |
context.Context
instance passed inContextEval()
can be propagated to binding function to cancel the process.First of all, thanks for the great OSS. We are using cel-go heavily in https://github.com/mercari/grpc-federation .
In our OSS, we may want to pass
context.Context
against bound functions. e.g.) pass to the gRPC metadata or perform a cancel.Now that the
ContextEval
function is provided, so I fixed to thecontext.Context
instance is passed to the bound function.Although I have tried to keep the Interface as unchanged as possible, I have made some changes to the Public Interface. If there is a problem, please let me know so that I can correct it.
ref #557
Pull Requests Guidelines
See CONTRIBUTING.md for more details about when to create
a GitHub Pull Request and when other kinds of contributions or
consultation might be more desirable.
When creating a new pull request, please fork the repo and work within a
development branch.
Commit Messages
Reviews
Merging
they have write access. Otherwise, the change will be merged by a maintainer.
closes #<issue-num>: description
in the merge message,if applicable.