add ManageActivity API #522
Conversation
| // Returns a `NotFound` error if there is no pending activity with the provided ID or type. | ||
| // (-- api-linter: core::0136::prepositions=disabled | ||
| // aip.dev/not-precedent: "By" is used to indicate request type. --) | ||
| rpc ManageActivity (ManageActivityRequest) returns (ManageActivityResponse) { |
There was a problem hiding this comment.
Can we reuse ExecuteMultiOperation? It is not named to be specific to workflows, and we don't surface it to the users as raw multi-op. Rather we make specific SDK things that use the generic multi-op. And server-side the multi-op can only support certain combinations if we want.
There was a problem hiding this comment.
This is already big move for me - I'm not a fan of "one function to rule them all" approach:)
But more important - I would rather have some separation of concerns.
I want to keep logical separation - it is hard to explain why we may want to bundle "unpause activity" and "start workflow". Also routing is different. Etc.
But separation of concerns is the main objection.
That said - if name is concern how about ExecuteMultipleActivityOperations?
(though ChatGPT doesn't like it, it thinks ManageActivity is concise and descriptive )
There was a problem hiding this comment.
I'm not a fan of "one function to rule them all" approach
But it was decided before I think. I do like ExecuteMultipleActivityOperation more than ManageActivity, but please run by the rest of server team. I think ExecuteMultipleOperation was meant for general purpose use despite personal preferences. But if team decides that it should have been named ExecuteMultipleWorkflowOperation and they made a mistake, ok.
There was a problem hiding this comment.
All current use-cases are workflow related. And it was not designed for other entities besides workflow.
There are multiple reasons why "One single API" is not ideal, and why having "one function per entity" is preferable
- discoverability.
- clarity. clear boundaries. separation of concerns
- validation/error handling.
- ortogonal routing parameters
- compatibility. Some combinations doesn't make sense, some can contradict each other
- cleaner code, easier to maintain.
- independent path forward
probably more.
I will also talk to more people, but so far I don't want to combine them - it is not ideal for too many reason
There was a problem hiding this comment.
no strong opinion on name though.
There was a problem hiding this comment.
All current use-cases are workflow related
It is clearly not named for only workflows unlike RPCs that are clearly named for workflows. Regardless of reason for avoiding "one single API", this was decided when that API was made. Not sure we should re-litigate for every use case or different committer.
There was a problem hiding this comment.
From my conversation with the team - no, we don't think about activity API as a part of it.
I put a lot of reasons why I think separate function is better then 'throw them all into single API".
So far haven't seen any "con" beside "it was decided".
There was a problem hiding this comment.
Inconsistency and disregarding past decisions are major cons. The next PR that comes about wanting to do it yet a third way (then fourth, then fifth) are effectively now encouraged to do so. Similar statements to "haven't seen any 'con'" and such will be said for a new approach each time the developer wants to avoid consistency.
There was a problem hiding this comment.
Inconsistency can't be an argument here - that existing function doesn't support any activity related operation. And was not designed to support. So we are not in "some activity operations are here, some are there" state.
"disregarding past decisions" can't be an argument "per se". If we make a decision to walk using our hands - proposal to walk using legs will obviously disregard that decision.
Our situation is even worth - we are not walking using our hands. Yet. But we are trying to enforce walking using hands, because we have some other decision, and "walking" somehow fits into it "by analogy". Thus is we decide to walk - we should walk using hands. I can't agree with that.
That said - I confirmed that we never design that function for activity operations. And in its current state it will not fit my use case (but this argument is already in the list of my other arguments).
There was a problem hiding this comment.
Inconsistency can't be an argument here - that existing function doesn't support any activity related operation. And was not designed to support
Disagree, it was clearly designed to support any number of calls to the oneof, even though it only has the initial calls to solve the initial use case, and was clearly not made to be workflow specific. This is a perfect use of it IMO.
What changed?
Add "ManageActivity" API.
Description:
ManageActivity apply reset/pause/unpause/update operations to an activity specified by its ID and/or type.
Either activity id or activity type must be provided. If activity type is specified - multiple activities can be affected.
Supported operations:
Pause operation. Pauses the activity. If activity was already paused it is no-op.
Reset operation. Resets the activity to its initial state.
This operation will reset the number of attempts and:
Flags:
'reset_heartbeats' indicates that activity should reset heartbeat details.
'keep_paused'- prevents activity from being unpaused.
Update operation - updates the activity options. For details see UnpauseActivityById.
Unpause operation. Unpauses the activity. If activity was not paused this will be a no-op.
If activity was waiting for retry - it will be scheduled immediately.
Unpause operation supports the following flags:
'jitter' - if set, the activity will start at a random time within the specified jitter duration.
ManageActivities returns a
NotFounderror if there is no pending activity with the provided ID or type.If multiple operations of the same type are provided the last one will be used.
If multiple operations are provided they will be executed in the following order:
Why?
Note: because "ManageActivity" supports multiple operations, each operation support only flags relevant to that operation.
Breaking changes
technically yes, but this is a separate branch.
Server PR
Irrelevant.