[Scheduled Actions V2] WIP: top-level Scheduler state machine #6904
Conversation
| ) | ||
|
|
||
| type ( | ||
| // The top-level scheduler state machine is compromised of 3 substate machines: |
There was a problem hiding this comment.
nit:
| // The top-level scheduler state machine is compromised of 3 substate machines: | |
| // The top-level scheduler state machine is compromised of 3 sub state machines: |
There was a problem hiding this comment.
will fix here/elsewhere
| // - Executor: executes buffered actions | ||
| // - Backfiller: buffers actions according to requested backfills | ||
| // | ||
| // A running scheduler will always have exactly one of each of the above substate |
There was a problem hiding this comment.
nit: is this space intentional?
There was a problem hiding this comment.
nope, will fix that.
| } | ||
| s2, ok := a.(Scheduler) | ||
| if !ok { | ||
| return 0, fmt.Errorf("%w: expected state1 to be a Scheduler instance, got %v", hsm.ErrIncompatibleType, s2) |
There was a problem hiding this comment.
| return 0, fmt.Errorf("%w: expected state1 to be a Scheduler instance, got %v", hsm.ErrIncompatibleType, s2) | |
| return 0, fmt.Errorf("%w: expected state2 to be a Scheduler instance, got %v", hsm.ErrIncompatibleType, s2) |
| if s1.State() > s2.State() { | ||
| return 1, nil | ||
| } else if s1.State() < s2.State() { | ||
| return -1, nil | ||
| } |
There was a problem hiding this comment.
This method is used to determine whether to sync a state machine across clusters in the soon-to-be-deprecated replication stack.
The way you've implemented this will prevent updates to the scheduler from being synced. You may be able to use the conflict token instead but that has some unwanted implications when there's a split brain.
Hopefully state based replication will be ready before the scheduler work and you won't have to implement this at all.
For now, I'd start with an implementation that panics and put a TODO here.
There was a problem hiding this comment.
For now, I'd start with an implementation that panics and put a TODO here.
Yep, will do.
| // | ||
| // When decrement is true, the schedule's state's `RemainingActions` counter | ||
| // is decremented and the conflict token is bumped. | ||
| func (s Scheduler) CanTakeScheduledAction(decrement bool) bool { |
There was a problem hiding this comment.
I would rename this to UseScheduledAction instead of having a side effect in a method named "Can...".
| if s.Schedule.State.RemainingActions > 0 { | ||
| if decrement { | ||
| s.Schedule.State.RemainingActions-- | ||
| s.ConflictToken++ |
There was a problem hiding this comment.
I don't think we want to increment the conflict token here, it can fail a user update request for no good reason.
There was a problem hiding this comment.
Agreed, will remove.
|
|
||
| func (s Scheduler) CompiledSpec(specBuilder *scheduler.SpecBuilder) (*scheduler.CompiledSpec, error) { | ||
| // cache compiled spec | ||
| if s.compiledSpec == nil { |
There was a problem hiding this comment.
Use the conflict token number to know if your cache may be invalid instead of worrying about manual invalidation?
There was a problem hiding this comment.
This isn't for manual invalidation, it's to support lazily loading the compiled spec after deserializing the state machine. Ideally, I'd fill this in at Deserialize time, but that would require I have access to the SpecBuilder (which itself caches time zone files from disk).. I suppose I could wire in SpecBuilder onto the machineDefinition, but I think I remember you mentioning that CHASM automates the Deserialize/Serialize bits away from components, right?
There was a problem hiding this comment.
What if the spec is updated later, don't you need to recompile it?
CHASM automates the Deserialize/Serialize bits away from components
Yes, which means you're right about not wanting to put this in deserialize, but also for the reason I gave above.
There was a problem hiding this comment.
What if the spec is updated later, don't you need to recompile it?
Yeah, you do; I see your point, will update.
| @@ -0,0 +1,171 @@ | |||
| package core | |||
There was a problem hiding this comment.
I wouldn't create this extra package, just put everything in components/scheduler, it'll help you avoid circular dependencies later and allow you to not expose anything that's not strictly necessary.
There was a problem hiding this comment.
The reason it's separated out is because the substate machines would have a lot of conflicts both between boilerplate (generator.MachineKey/backfiller.MachineKey -> GeneratorMachineKey/BackfillerMachineKey) as well as conflict between states/events (generator.TransitionSleep/executor.TransitionSleep -> TransitionGeneratorSleep/TransitionExecutorSleep). It also helps avoid creating circular dependencies and tight coupling between components.
I can be convinced the other way if it's going to be the standard for HSM projects, to keep all within a single package, but if there is a component that were to be fitting for multiple component packages, I think it'd be this one.
There was a problem hiding this comment.
Hmm... will leave it up to you.
Based on #6903
What changed?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?