Improvement the Protocol #8
Conversation
sleipnir
added
enhancement
sleipnir
requested review from
marcellanz and
wesleimp
and removed request for
marcellanz and
wesleimp
|
Thanks Adrinao, I'll have a look. |
| // See Command description Above | ||
| Command command = 2; | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion:
// Similar to TimerCommand but only executes commands following the Cron Expression
message CronCommand {
// The expression the cron execution will follow, example: "* 5 * * *"
string expression = 1;
// See Command description Above
Command command = 2;
}
There was a problem hiding this comment.
The problem with Timers and Crons is that the user needs to remember that Spawn Actors by default live a specific period of time before they deactivate. So they need to set a timeout for deactivation longer than the Timer or Cron timeout, which sometimes becomes complicated to configure especially with Cron's proposal.
I see that we will have to create a new deactivation strategy that is possible to use when the user is going to use Timers or Crons, probably an infinite time strategy, that is, that never deactivates an actor that has declared timers or crons.
| ActorSnapshotStrategy snapshot_strategy = 6; | ||
|
|
||
| // Deactivate strategy | ||
| ActorDeactivateStrategy deactivate_strategy = 7; |
There was a problem hiding this comment.
WDYT of grouping snapshot, deactivate and persistent in a single interface called persistence?
Also WDYT of naming ActorSnapshotStrategy to ActorStateStoreStrategy
message ActorPersistence {
bool (definitive or persistent) = 1;
ActorSnapshotStrategy snapshot_strategy = 2;
ActorDeactivateStrategy deactivate_strategy = 3;
}
There was a problem hiding this comment.
It looks good to me. I will add
There was a problem hiding this comment.
Could we use ActorDeactivationStrategy instead?
There was a problem hiding this comment.
Could we use ActorDeactivationStrategy instead?
I thought more about it and actually snapshot strategies are needed for actor state management whereas deactivation strategies are needed for actor lifecycle management. I think they are different things that if they were grouped together should have a neutral name.
WDYT @marcellanz and @eliasdarruda
There was a problem hiding this comment.
Maybe renaming it as ActorLifecycleStrategy as you described then?
There was a problem hiding this comment.
Maybe renaming it as
ActorLifecycleStrategyas you described then?
I just submitted a new commit I grouped these types inside a new type which I called ActorConfiguration. All right for you @eliasdarruda and @marcellanz ?
Thank you for your comments |
|
The last commits that entered the main branch evolved most of the needs of this PR. So I will close this PR and treat the other parts in other PRs. |
No description provided.