Copilot commented on code in PR #963:
URL: https://github.com/apache/dubbo-go-pixiu/pull/963#discussion_r3371170497
##########
pkg/filter/llm/proxy/filter.go:
##########
@@ -133,9 +137,12 @@ type (
}
)
-// sharedCooldownStore keeps endpoint cooldowns process-wide so filter reloads
-// and multiple LLM proxy factories do not reset runtime failure state.
-var sharedCooldownStore = newCooldownStore()
+// newPlugin builds the plugin with its process-wide cooldown store, so every
+// filter factory the plugin creates shares one runtime failure state that
+// survives individual factory reloads.
+func newPlugin() *Plugin {
+ return &Plugin{cooldowns: newCooldownStore()}
+}
Review Comment:
The PR description states the Plugin “lazily owns a single cooldown store
(sync.Once)”, but the implementation eagerly allocates a new store in
newPlugin() and does not use sync.Once. Either adjust the implementation to
match the described lazy/sync.Once ownership model, or update the PR
description to reflect the eager initialization so reviewers/operators aren’t
misled about lifecycle/overhead.
##########
pkg/filter/llm/proxy/filter.go:
##########
@@ -157,9 +164,11 @@ func (p *Plugin) Kind() string {
return Kind
}
-// CreateFilterFactory creates a new factory instance for this filter.
+// CreateFilterFactory creates a new factory instance for this filter. The
+// plugin-owned cooldown store is injected here so every factory and the
+// request executors it builds share one explicit store with no global
fallback.
func (p *Plugin) CreateFilterFactory() (filter.HttpFilterFactory, error) {
- return &FilterFactory{cfg: &Config{}}, nil
+ return &FilterFactory{cfg: &Config{}, cooldowns: p.cooldowns}, nil
}
Review Comment:
CreateFilterFactory injects p.cooldowns directly. If a Plugin is ever
constructed as a zero value (&Plugin{}), p.cooldowns will be nil and the
request path will silently disable cooldown tracking (executor/filter will
carry a nil store). To uphold the “executor always receives a non-nil store
from construction” contract without requiring callers to use newPlugin(),
initialize the store here when missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
