[KS-616] asset job updates #15573
Merged
[KS-616] asset job updates #15573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
krehermann
requested review from
Atrax1,
jmank88 and
cedric-cordenier
and removed request for
Atrax1
krehermann
force-pushed
the
ks-616/update-asset-don-jobs
branch
from
687b2fc to
670c0b3
Compare
jmank88
previously approved these changes
Dec 9, 2024
Flakeguard Summary
Found Flaky Tests ❌
|
Flakeguard Summary
Found Flaky Tests ❌
|
bolekk
previously approved these changes
Dec 10, 2024
plugins/loop_registry.go
Outdated
| @@ -45,14 +42,15 @@ func NewLoopRegistry(lggr logger.Logger, tracing config.Tracing, telemetry confi | |||
| } | |||
| } | |||
|
|
|||
| // Register creates a port of the plugin. It is not idempotent. Duplicate calls to Register will return [ErrExists] | |||
| // Register creates a port of the plugin. It is idempotent. Duplicate calls to Register will return the same port. | |||
There was a problem hiding this comment.
Any possible unintended consequences?
Also why would anybody call it more than once?
plugins/loop_registry.go
Outdated
| if _, exists := m.registry[id]; exists { | ||
| return nil, ErrExists | ||
| if l, exists := m.registry[id]; exists { | ||
| m.lggr.Warnf("Trying to register a loop that is already registered %q", id) |
There was a problem hiding this comment.
Hmmmm -- is this all we need to do? I would have thought that there would now be multiple loopps running, one of which is orhpaned because I don't see us cleaning it up.
There was a problem hiding this comment.
i believe it was a little different.
we are passing the loop server backas one of the job ServiceCtx objects and that will be closed when the active job closes all it's subservices
rather, we are forgetting to unregister the loop as part of this job close'ing
i refactored to make a much more targeted fix that only applies to mercury and a follow up ticket to make the pattern reusable for other jobs.
note that the generic jobs are already unregisteringand this should only effect job that really on custom providers
cedric-cordenier
approved these changes
Dec 10, 2024
| // loopUnregisterCloser is a helper to unregister a loop | ||
| // as a service | ||
| // TODO BCF-3451 all other jobs that use custom plugin providers that should be refactored to use this pattern | ||
| // perhaps it can be implemented in the delegate on job delete. |
bolekk
approved these changes
Dec 10, 2024
bolekk
approved these changes
Dec 10, 2024
patrickhuie19
approved these changes
Dec 10, 2024
jmank88
approved these changes
Dec 10, 2024
jmank88
approved these changes
Dec 10, 2024
samsondav
approved these changes
Dec 10, 2024
Merged
This was referenced Dec 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
KS-616
Error observed when updating job but not when creating.
Traced to duplicate registration in the Loop Registry.
This was then traced to missing Unregister when the Job closes all it's sub services.
Requires
Supports