Add status fields to Substrate resources - #1025
Conversation
2864483 to
9dd365f
Compare
|
Nevermind, I see.
|
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
LGTM overall - as you said "mechanical"
| // once at creation and immutable afterward. | ||
| ObjectRef source_snapshot_tag = 6; | ||
|
|
||
| ActorStatus status = 7; |
There was a problem hiding this comment.
Should we give status a high tag number (e.g. 1024)? Presumably we are going to add more fields in these resources. ISTR the tags are encoded as varint, so that has some downside...
There was a problem hiding this comment.
Is there any value in doing this? It's a single field. I've seen this being done in oneof fields so you can carve out a range for the oneof fields, but not sure about what value would that bring here.
| } | ||
|
|
||
| message ActorStatus { | ||
| enum State { |
There was a problem hiding this comment.
Aside: the resulting Go names for these are awful - WDYT about moving this to a file-level ActorState ?
There was a problem hiding this comment.
Done
| string actor_template_uid = 7; | ||
| SnapshotContentScope content_scope = 8; | ||
| string snapshot_uri = 9; | ||
| // Immutable reference to the actor_template_version where the snapshot was created from. |
There was a problem hiding this comment.
You lost the only comment in this whole section
There was a problem hiding this comment.
Added it back. It was intentional, all comments in ateapi.proto are pretty bad and I'm thinking about a PR dedicated just to documenting the API but I don't think it hurts.
| 3. **Hydration**: The `atelet` supervisor coordinates with the `ateom` process inside the worker pod to restore the ActorTemplate's golden `ActorSnapshot` (for first-run) or the Actor's latest `ActorSnapshot` (for recurring runs) into the sandbox. | ||
|
|
||
| 4. **Status**: Status transitions to `STATUS_RUNNING`. The actor now has an | ||
| 4. **Status**: Status transitions to `STATE_RUNNING`. The actor now has an |
There was a problem hiding this comment.
Done.
| if actor.Status != ateapipb.Actor_STATUS_SUSPENDED { | ||
| t.Errorf("expected actor status to be SUSPENDED, got %v", actor.Status) | ||
| if actor.Status.State != ateapipb.ActorStatus_STATE_SUSPENDED { | ||
| t.Errorf("expected actor status to be SUSPENDED, got %v", actor.Status.State) |
There was a problem hiding this comment.
state
I found a few of these, I am sure it's not all of them. Maybe git grep -i 'actor.*status' and see what hits?
There was a problem hiding this comment.
Done. Thanks for the command hint.
| 3. **Reclaim**: The physical worker is wiped and returned to the `WorkerPool`. | ||
|
|
||
| 4. **Status**: Status transitions back to `STATUS_SUSPENDED`, now pointing to | ||
| 4. **Status**: Status transitions back to `STATE_SUSPENDED`, now pointing to |
There was a problem hiding this comment.
Done.
| // phase string the UI's badge logic understands (running / suspended | ||
| // / etc). | ||
| func actorStatusString(s ateapipb.Actor_Status) string { | ||
| func actorStatusString(s ateapipb.ActorStatus_State) string { |
There was a problem hiding this comment.
actorStateString
There was a problem hiding this comment.
Done
9dd365f to
00456ff
Compare
|
FYI: I've added one more commit for ActorTemplate as Zoe Zhao (@zoez7) changes are merged already. |
3 participants
Add
statusfields to all Substrate resources that need it. Move server-owned fields under it.Fixes #1006 .