Conversation
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
| # When true, secondary shards load assigned workflows and cache trigger events without executing. | ||
| # The primary shard sends ExecutionCompleted notifications so the secondary can drain its cache. | ||
| # Requires ShardingEnabled=true. | ||
| ShardingFailoverEnabled = false # Default |
There was a problem hiding this comment.
Can we move this to cresettings as a feature flag? It makes sense to enable on all nodes at the same time anyway + it will be easier to manage.
There was a problem hiding this comment.
it's a good idea, I took a note and we can change this as a separate PR
| MethodTriggerEvent = "TriggerEvent" | ||
| MethodExecute = "Execute" | ||
| MethodTriggerEventAck = "TriggerEventACK" | ||
| MethodExecutionCompleted = "ExecutionCompleted" |
There was a problem hiding this comment.
similarly to my comment on the proto - I'd name it "ExecutionStatusUpdate"
There was a problem hiding this comment.
yep, I will refactor in next iteration over whole. let's make it work and then we can make it beautiful ;)
| donB.AcceptsWorkflows && len(donA.CapabilityConfigurations) > 0 { // add DON pair if B is workflow and A is capability | ||
| if donA.AcceptsWorkflows && len(donB.CapabilityConfigurations) > 0 || // add DON pair if A is workflow and B is capability | ||
| donB.AcceptsWorkflows && len(donA.CapabilityConfigurations) > 0 || // add DON pair if B is workflow and A is capability | ||
| donA.AcceptsWorkflows && donB.AcceptsWorkflows { // shard-to-shard for ExecutionCompleted failover delivery |
There was a problem hiding this comment.
ha! this turned out to be super easy :)
There was a problem hiding this comment.
yes it looks like that, but I remember don2don filtering so I'm a bit skeptical until properly tested
| expiryDuration time.Duration | ||
| } | ||
|
|
||
| func NewExecutionCompletedReceiver(primary commoncap.DON, handler ExecutionCompletedHandler, lggr logger.Logger) *ExecutionCompletedReceiver { |
There was a problem hiding this comment.
Are receiver and sender wired into d2d comms at all? If not, maybe move them to a separate PR with the wiring?
|
|
|
||
| primaryAssignmentTOML := ` | ||
| static_default_assignment = [0,1] | ||
| hashed_default_assignment = false |
There was a problem hiding this comment.
wen test for hashed assignment?
There was a problem hiding this comment.
that's ring activation as default, so it's already covered and tests exist
|
|
||
| [per_org_assignment] | ||
| org_test_failover = [1,0] | ||
| ` |
There was a problem hiding this comment.
how about adding a comment in the test explaining what this modified jobspec does? two settings were modified here, I think it would be useful to have some context about their effect (as the intent is clear: workflow swapping... but why for example we need to modify both values?)
| workflowIDs[i] = workflowID | ||
| workflowToShardIndex[workflowID] = 1 | ||
| } | ||
| testLogger.Info().Strs("workflowIDs", workflowIDs).Msg("Deployed fresh workflows for swap phase") |
There was a problem hiding this comment.
I am missing explanatory comments here, e.g.
- what is supposed to happen with the original workflows deployed to shard0?
- why do we need to swap them with other ones?
- why we don't wait for newly deployed workflows to execute before we propose a new job?
Looking at the test naively I would just expect to see original workflows executing on shard1 after the failover, but here something more complex is going on and I don't really understand what.
3 participants
added:
ExecutionCompletedandShardHeartbeatDon2Don protobuf messages with F+1 quorum-based senders/receivers in a new core/capabilities/remote/sharding/ packageOnExecutionCompletedlifecycle hook into the engine's deferred execution blockShardingFailoverEnabledconfig flag that makes non-primary shards load assigned workflows and cache trigger events without executingAllShardsResolverinterface so the registry loads workflows on all assigned shardsTest_CRE_V2_FailoverManualSwapsystem testdep. smartcontractkit/chainlink-protos#450
cre-4799