Validate dependencies for all producer/consumer plugins in datalayer#2246
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
size/M
|
/assign @kfswain |
rahulgurnani
force-pushed
the
datalayer-rework
branch
from
8d77ba9 to
5803013
Compare
k8s-ci-robot
added
size/L
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
rahulgurnani
force-pushed
the
datalayer-rework
branch
from
f0f32c0 to
09ad86f
Compare
rahulgurnani
force-pushed
the
datalayer-rework
branch
from
09ad86f to
7a24f41
Compare
|
anything blocking this? |
pkg/epp/requestcontrol/dag.go
Outdated
| for producedKey, producedData := range plugins[i].Produces() { | ||
| if producer.Produces() != nil && consumer.Consumes() != nil { | ||
| for producedKey, producedData := range producer.Produces() { | ||
| // If plugin j consumes the produced key, then j depends on i. We can break after the first match. |
There was a problem hiding this comment.
lets update these comments to the new var names
There was a problem hiding this comment.
Thanks for the catch. The new variable names are quite intutive. I think we don't need some of these comments now. Removed them.
| for producedKey, producedData := range producer.Produces() { | ||
| // If plugin j consumes the produced key, then j depends on i. We can break after the first match. | ||
| if consumedData, ok := plugins[j].Consumes()[producedKey]; ok { | ||
| if consumedData, ok := consumer.Consumes()[producedKey]; ok { |
There was a problem hiding this comment.
We should also check that plugins arent attempting to consume data from a layer they dont have access to. example: Flow Control plugins currently can't rely on Request Specific data
There was a problem hiding this comment.
I think, that's something we should do in a separate change. AFAIK, we presently don't have the information about which layer the plugin is from here etc. It may require significant redesign and refactor. Added a TODO for it now. Thanks!
There was a problem hiding this comment.
SG, spun up a an issue, added it to the milestone & assigned to you. Thanks!
| } | ||
|
|
||
| // orderPrepareDataPlugins reorders the prepareDataPlugins in the Config based on the given sorted plugin names. | ||
| func (c *Config) orderPrepareDataPlugins(sortedPluginNames []string) { |
There was a problem hiding this comment.
Is this consumed in a way that parallelizes preparedata steps until it hits a plugin that has a dependency?
If not, we should add that, I think.
There was a problem hiding this comment.
no.. I had added the code to parallelize it earlier but based on Nir's suggestion, I kept the plugin execution sequential for now for simplicity. I did check the performance differences and prepare data step execution latency in parallel vs sequential had hardly any difference. I think we can stick with sequential execution in the correct order (topologically sorted order) for now.
Reference:
#1878 (comment)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain, rahulgurnani The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…ubernetes-sigs#2246) * Validate all producer consumer plugins * Rename methods, cleanup * More cleanup * Remove stale comments * Add TODO for pool-level/request level validations
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Presently only prepare data plugins are validated for dependency check for cyclic dependencies. In this change, the check is extended to all producer/consumer plugins.
Which issue(s) this PR fixes:
Fixes #1988
Does this PR introduce a user-facing change?: