[CRE-941] Dynamic config updates in TriggerPublisher + ExecutableClient/Server #19702
Conversation
|
I see you updated files related to
|
bolekk
force-pushed
the
feature/CRE-941-dynamic-shim-updates
branch
2 times, most recently
from
f35221b to
3f734c3
Compare
bolekk
changed the title
bolekk
force-pushed
the
feature/CRE-941-dynamic-shim-updates
branch
3 times, most recently
from
6060e0c to
190a3de
Compare
product-security-plaid-production
bot
requested review from
DylanTinianov and
krehermann
|
|
||
| func (c *client) Start(ctx context.Context) error { | ||
| return c.StartOnce(c.Name(), func() error { | ||
| cfg := c.cfg.Load() |
There was a problem hiding this comment.
i think this is non-recoverable.
if you pass a bad config, and call start, then call setConfig with a good config, and call start does it start?
i don't think it will. is there a test?
There was a problem hiding this comment.
all make a typed error for the caller to check if you want them to handle it ~
var ErrConfigNotSet = errors.New(...)
...
return fmt.Errorf("%w: empty remote capabilty", ErrConfigNotSet)
++ caller ++
err : = x.Start()
if errors.Is(ErrConfigNotSet) {
x.SetConfig
}
There was a problem hiding this comment.
Launcher will cache the shim only when Start() succeeded. Otherwise it will be re-created on each iteration with fresh config. It wasn't implemented consistently for all 4 cases but I now fixed it and added comments to point it out. Thanks!
| if errCfg := execCap.SetConfig(info, myDON.DON, defaultTargetRequestTimeout, nil); errCfg != nil { | ||
| return nil, fmt.Errorf("failed to set trigger config: %w", errCfg) | ||
| } | ||
| return execCap.(capabilityService), nil |
There was a problem hiding this comment.
type check here? in prinicple can panic and kill the node
x, ok := exec.Cap.(capabilityService)
if !ok {
return nil, fmt.Errorf("failed to convert a into b...")
}
return x, nil
There was a problem hiding this comment.
Feels a bit excessive. Panic will be caught by unit tests, if the actual object is ever changed to not implement the interface any more.
bolekk
force-pushed
the
feature/CRE-941-dynamic-shim-updates
branch
from
190a3de to
43e8ccd
Compare
| wg sync.WaitGroup | ||
| } | ||
|
|
||
| type dynamicConfig struct { |
There was a problem hiding this comment.
maybe some sort of "version" tag field could make it easier in debugging and logs, knowing which config are we currently dealing with application wide?
There was a problem hiding this comment.
We don't store any version onchain so it's not really possible to map it to anything sensible here.
Launcher calls SetConfig() on every iteration, even if values didn't change so an update timestamp also doesn't make too much sense...
| return errors.New("requestTimeout must be positive") | ||
| } | ||
| // Use a sensible default if maxParallelRequests is not set or is 0 | ||
| if maxParallelRequests <= 0 { |
There was a problem hiding this comment.
Setting this will have no impact once the server is started, ie it is not dynamically settable. This is intentional?
There was a problem hiding this comment.
Thanks for pointing this out. Making it dynamic will be a PITA and it's also not very important... I added a warning log for now.
bolekk
force-pushed
the
feature/CRE-941-dynamic-shim-updates
branch
from
43e8ccd to
cb82ba6
Compare
bolekk
force-pushed
the
feature/CRE-941-dynamic-shim-updates
branch
2 times, most recently
from
4b7a76f to
df4672f
Compare
bolekk
force-pushed
the
feature/CRE-941-dynamic-shim-updates
branch
from
df4672f to
0ca8226
Compare
|
4 participants
Follow up to #19404, which covers all remaining remote shims.
Move most config out of constructors and into setters.