-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add activiy APIs - pause, reset, resume, describe #456
base: master
Are you sure you want to change the base?
Conversation
// Activity options after an update | ||
temporal.api.activity.v1.ActivityOptions activity_options = 1; | ||
|
||
// Activity state | ||
temporal.api.enums.v1.ActivityState activity_state = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is value in temporal.api.activity.v1.ActivityDescription
that encompasses these and maybe more in the future. Arguably this API and pending activity API have a lot of model overlap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does has a lot of in common with PendingActivityInfo. And I wish I can just use PendingActivityInfo. But - PendingActivityInfo is more "internal". It has lots of fields that are at best "situational", etc.
I though it will be cleaner if we have a separate structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring the pending activity info part, I still wonder if a combination of these things (and potentially more in the future) deserves an encompassing ActivityDescription
message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like "DescribeWorkflowExecutionResponse", I think this structure is the one to have all potential fields, and wrapping those fields into ActivityDescription message doesn't add much. It is an extra level of indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
The only reason I am hesitant to approve is I would like to see if we can get implementation (mostly) done before merging just in case we learn things during implementation. But otherwise, effectively "approved" from my POV.
@@ -871,5 +870,71 @@ service WorkflowService { | |||
}; | |||
} | |||
|
|||
} | |||
// DescribeActivityById is called by the client to get current activity options and state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only return pending activities? Otherwise, how does this handle when two activities with the same ID (yes, that's supported as long as they're not both pending).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking because DescribeWorkflowExecution only returns pending activities as those are stored in mutable state. If you want to return finished activities, you'll need to load history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only pending activities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this in the docstring.
// If activity was paused, it will be kept paused after reset | ||
bool keep_paused = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support reset and pause in the same request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like we're still not aligned here, as I still am worried about unpausing being the default.
To give us time to collect more opinions, can we leave it out for this PR and consider adding it later after consultation with folks like Roey/Max/Chad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support reset and pause in the same request?
I think they are different operations entirely. Whether we want to combine operations or not in general I don't have a strong opinion, but I don't think these two are special.
To give us time to collect more opinions, can we leave it out for this PR
Can you clarify which part to leave out of the PR? Pausing? Reset? If both are still in, we need to define some behavior for resetting a paused activity and a default. Whether that default is unpause or remain paused I have no opinion on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is after long discussions with Drew. Basically if user paused and then reset - in some cases he may want to keep paused activities to stay paused. We right now discussing what should be "by default" - keep paused and specify a flag to resume, or resume and specify a flag to keep paused.
@@ -1695,3 +1696,92 @@ message UpdateActivityOptionsByIdResponse { | |||
// Activity options after an update | |||
temporal.api.activity.v1.ActivityOptions activity_options = 1; | |||
} | |||
|
|||
message DescribeActivityByIdRequest { | |||
// Namespace of the workflow which scheduled this activity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put .
at the end of sentences to be consistent with the rest of the docstrings in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping UpdateActivityOptionsByIdRequest (which doesn't have dots for the same reason), scrolling above XXXNexus request/responses:
PollWorkflowExecutionUpdateResponse - no dots
PollWorkflowExecutionUpdateRequest - no dots
ListBatchOperationsResponse - no dots
ListBatchOperationsRequest - no dots
DescribeBatchOperationResponse - no dots
StopBatchOperationRequest - no dots
activity related:
PollActivityTaskQueueResponse - no dots
RecordActivityTaskHeartbeatRequest - no dots
RecordActivityTaskHeartbeatByIdRequest- no dots
from what I see not having .
at the end is more consistent with the rest of the docstrings in this file.
I do also see some with dots, but seems they are minority.
Though if we go top-to-bottom - first few have dots...
Hm, also it looks like this is a subject of some heated discussions online. And Google proto style guidelines don't have recommendations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have periods in our doc strings. I think the majority do.
ACTIVITY_STATE_UNSPECIFIED = 0; | ||
ACTIVITY_STATE_SCHEDULED = 1; | ||
ACTIVITY_STATE_STARTED = 2; | ||
ACTIVITY_STATE_PAUSED = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not loving PAUSED as a separate state, I prefer to model pause as a separate dimension to be able to express things like "started and paused", which is a valid state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do think that PAUSED is a valid state, please update PendingActivityState
to reflect this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add PAUSED and BACKING_OFF to PendingActivityState
+1 to this, please use a feature branch until the implementation solidifies and you're confident in the API. |
required: true | ||
schema: | ||
type: string | ||
- name: workflowId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be required? If not, can comments explain the requirements ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is auto-generated code for the HTTP API and is unrelated to whether something is required from the gRPC side. The generator only marks things as required that are present in the URL path and we had decided in the past that activity RPC URLs are not sub-URLs of the workflow. We may be able to change that though if we want (or add additional bindings). We may also be able to look in to validation annotations to be more specific about which fields are required in proto, but that is a general/separate topic and not specific to this PR.
// Id of the activity we're updating | ||
string activity_id = 4; | ||
|
||
// The identity of the client who initiated this request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
Is this a freeform user-specified field, or otherwise, how does somebody obtain one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "client identity" is present in almost all actionable RPCs and is a unique client identifier sent by the SDK. It allows you to uniquely identify a client that did something (we often store this on events and other places). This is a settable option in every SDK and usually defaults to a string with process ID and hostname.
// If activity was paused, it will be kept paused after reset | ||
bool keep_paused = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like we're still not aligned here, as I still am worried about unpausing being the default.
To give us time to collect more opinions, can we leave it out for this PR and consider adding it later after consultation with folks like Roey/Max/Chad?
Add definitions for DescribeActivity, PauseActivity, ResumeActivity and ResetActivity API
Part of Activity API work.
No.
Server PR
temporalio/temporal#6584