-
Notifications
You must be signed in to change notification settings - Fork 180
feat: Added a factory function for the DecisionTree filter #1053
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
Changes from all commits
9e6223b
b88054e
5e38e72
293a4f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,13 +18,21 @@ package filter | |
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"errors" | ||
"fmt" | ||
|
||
"sigs.k8s.io/controller-runtime/pkg/log" | ||
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/plugins" | ||
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/framework" | ||
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/types" | ||
logutil "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/util/logging" | ||
) | ||
|
||
const ( | ||
DecisionTreeFilterType = "decision-tree" | ||
) | ||
|
||
// compile-time type assertion | ||
var _ framework.Filter = &DecisionTreeFilter{} | ||
|
||
|
@@ -47,6 +55,82 @@ type DecisionTreeFilter struct { | |
NextOnSuccessOrFailure framework.Filter | ||
} | ||
|
||
type decisionTreeFilterParameters struct { | ||
Current *decisionTreeFilterEntry `json:"current"` | ||
NextOnSuccess *decisionTreeFilterEntry `json:"nextOnSuccess"` | ||
NextOnFailure *decisionTreeFilterEntry `json:"nextOnFailure"` | ||
NextOnSuccessOrFailure *decisionTreeFilterEntry `json:"nextOnSuccessOrFailure"` | ||
} | ||
|
||
type decisionTreeFilterEntry struct { | ||
PluginRef *string `json:"pluginRef"` | ||
DecisionTree *decisionTreeFilterParameters `json:"decisionTree"` | ||
} | ||
|
||
func DecisionTreeFilterFactory(name string, rawParameters json.RawMessage, handle plugins.Handle) (plugins.Plugin, error) { | ||
parameters := decisionTreeFilterParameters{} | ||
if err := json.Unmarshal(rawParameters, ¶meters); err != nil { | ||
return nil, fmt.Errorf("failed to parse the parameters of the '%s' filter - %w", name, err) | ||
} | ||
return loadDecisionTree(¶meters, handle) | ||
} | ||
|
||
func loadDecisionTree(parameters *decisionTreeFilterParameters, handle plugins.Handle) (*DecisionTreeFilter, error) { | ||
result := &DecisionTreeFilter{} | ||
var err error | ||
|
||
if parameters.Current == nil { | ||
return nil, errors.New("a current filter must be specified") | ||
} | ||
result.Current, err = loadDecisionTreeEntry(parameters.Current, handle) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if parameters.NextOnSuccess != nil { | ||
result.NextOnSuccess, err = loadDecisionTreeEntry(parameters.NextOnSuccess, handle) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
if parameters.NextOnFailure != nil { | ||
result.NextOnFailure, err = loadDecisionTreeEntry(parameters.NextOnFailure, handle) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
if parameters.NextOnSuccessOrFailure != nil { | ||
result.NextOnSuccessOrFailure, err = loadDecisionTreeEntry(parameters.NextOnSuccessOrFailure, handle) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
return result, nil | ||
} | ||
|
||
func loadDecisionTreeEntry(entry *decisionTreeFilterEntry, handle plugins.Handle) (framework.Filter, error) { | ||
if entry.PluginRef != nil && entry.DecisionTree != nil { | ||
return nil, errors.New("both pluginRef and decisionTree may not be specified") | ||
} | ||
|
||
if entry.PluginRef != nil { | ||
instance := handle.Plugins().Plugin(*entry.PluginRef) | ||
if instance == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unrelated but I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++, agree with one small difference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. map[key], doesn't have to return a bool. There were reviews in which w were told to remove the bool and check for nll instead. In general getters in Go do not have the prefix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more idiomatic in go to have the Just calling this out but not related to this PR thought |
||
return nil, errors.New(*entry.PluginRef + " is a reference to an undefined Plugin") | ||
} | ||
if theFilter, ok := instance.(framework.Filter); ok { | ||
return theFilter, nil | ||
} | ||
return nil, errors.New(*entry.PluginRef + " is not a filter") | ||
} else if entry.DecisionTree != nil { | ||
return loadDecisionTree(entry.DecisionTree, handle) | ||
} | ||
return nil, errors.New("either pluginRef or decisionTree must be specified") | ||
} | ||
|
||
// Type returns the type of the filter. | ||
func (f *DecisionTreeFilter) Type() string { | ||
if f == nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same nit, return concrete type
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't always return a DecisionTreeFilter, it may return a framework.Filter.