Workflow backends: ignore on hot reloading #7433
Merged
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
| func (wfbe *workflowBackend) Init(ctx context.Context, comp compapi.Component) error { | ||
| wfbe.lock.Lock() | ||
| defer wfbe.lock.Unlock() | ||
| func (w *workflowBackend) Init(ctx context.Context, comp compapi.Component) error { |
There was a problem hiding this comment.
Please don't rename variables for no reason, it causes unnecessary merge conflicts
There was a problem hiding this comment.
By convention we use single letter variable names for method receiver variables. This gives overhead to the reader as it is unexpected. wfbe initially tripped my up as I was confused as to where this variable was coming from until I realized it was the receiver.
| wg.Wait() | ||
|
|
||
| errs := make([]error, 0, len(group.resources)) | ||
| for range group.resources { |
There was a problem hiding this comment.
Suggested change
| for range group.resources { | |
| for i := 0; i < len(group.resources); i++ { |
There was a problem hiding this comment.
What is the benefit of creating the variable i here?
| @@ -48,7 +49,7 @@ type ComponentStore struct { | |||
| pubSubs map[string]PubsubItem | |||
| topicRoutes map[string]TopicRoutes | |||
| workflowComponents map[string]workflows.Workflow | |||
| workflowBackends map[string]wfbe.WorkflowBackend | |||
| workflowBackends map[string]backend.Backend | |||
There was a problem hiding this comment.
This is the correct type for the workflow backend implementation interface. Notice the other component types are using the interface types defined in components-contrib however backends is using durabletask-go directly. This wasn't picked up before as this field wasn't previously being used (workflowbackend.go didn't exist). wfbe.WorkflowBackend is a factory func type.
artursouza
added
the
autoupdate
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #7433 +/- ##
==========================================
- Coverage 62.30% 62.29% -0.01%
==========================================
Files 244 244
Lines 22203 22210 +7
==========================================
+ Hits 13833 13836 +3
Misses 7232 7232
- Partials 1138 1142 +4 ☔ View full report in Codecov by Sentry. |
JoshVanL
added a commit
to JoshVanL/dapr
that referenced
this pull request
Jan 24, 2024
Updates the hot reloading reconciler so that Daprd will exit error when a actor state store enabled Component is hot reloaded. This is chosen because today, the actors subsystem is not written with any closing or dynamic support. Doing so will cause panics/corruption in its current state. Exiting error is the safest option as this ensures consistency across a replica set and ensures there is no surprise for the user that behaviour does not match given configuration. See also dapr#7433 Signed-off-by: joshvanl <me@joshvanl.dev>
JoshVanL
added a commit
to JoshVanL/dapr
that referenced
this pull request
Jan 25, 2024
Updates the hot reloading reconciler so that Daprd will exit error when a actor state store enabled Component is hot reloaded. This is chosen because today, the actors subsystem is not written with any closing or dynamic support. Doing so will cause panics/corruption in its current state. Exiting error is the safest option as this ensures consistency across a replica set and ensures there is no surprise for the user that behaviour does not match given configuration. See also dapr#7433 Signed-off-by: joshvanl <me@joshvanl.dev>
JoshVanL
force-pushed
the
hotreloading-workflowbackends-exit1
branch
from
4450de0
to
e05b4b9
Compare
Open
Updates the hot reloading reconciler so that Daprd will exit error when a workflowbackend Component is hot reloaded. This is chosen because today, the actors and workflow subsystems are not written with any closing or dynamic support. Doing so will cause panics/corruption in its current state. Exiting error is the safest option as this ensures consistency across a replica set and ensures there is no surprise for the user that behaviour does not match given configuration. Adds integration tests for daprd ensuring workflow backends can be loaded on boot. Adds tests to ensure daprd will exist error on a workflow backend being hot reloaded. We should do the same for actor state store and the actor subsystem. Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: joshvanl <me@joshvanl.dev>
registered Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: joshvanl <me@joshvanl.dev>
JoshVanL
force-pushed
the
hotreloading-workflowbackends-exit1
branch
from
e05b4b9
to
ba58da7
Compare
reconciled Signed-off-by: joshvanl <me@joshvanl.dev>
JoshVanL
changed the title
yaron2
pushed a commit
that referenced
this pull request
Jan 30, 2024
* Exit error if actor state store hot reloaded Updates the hot reloading reconciler so that Daprd will exit error when a actor state store enabled Component is hot reloaded. This is chosen because today, the actors subsystem is not written with any closing or dynamic support. Doing so will cause panics/corruption in its current state. Exiting error is the safest option as this ensures consistency across a replica set and ensures there is no surprise for the user that behaviour does not match given configuration. See also #7433 Signed-off-by: joshvanl <me@joshvanl.dev> * Remove expected workflow.dapr component from actorastate tests Signed-off-by: joshvanl <me@joshvanl.dev> * Increase assert Eventually timeout Signed-off-by: joshvanl <me@joshvanl.dev> * Only log error and don't exit error when reocnciling actor state store Signed-off-by: joshvanl <me@joshvanl.dev> --------- Signed-off-by: joshvanl <me@joshvanl.dev> Signed-off-by: Artur Souza <asouza.pro@gmail.com> Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
artursouza
previously approved these changes
Jan 30, 2024
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
artursouza
approved these changes
Jan 30, 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.
Updates the hot reloading reconciler so that Daprd will log and error and ignore workflow backends when they are hot reloaded.
Adds integration tests for daprd ensuring workflow backends can be loaded on boot.
Adds tests to ensure daprd will log an error on a workflow backend being hot reloaded.
Adds tests to ensure multiple workflow backends cannot be loaded at once.
closes #7409