RPC boiler plate and code generation#9693
Conversation
spkane31
left a comment
There was a problem hiding this comment.
These are all the places I actually changed, the rest are code gen
| } | ||
|
|
||
|
|
||
| message PauseActivityExecutionRequest { |
There was a problem hiding this comment.
Duplicating existing requests
| option (temporal.server.api.common.v1.api_category).category = API_CATEGORY_STANDARD; | ||
| } | ||
|
|
||
| rpc PauseActivityExecution(PauseActivityExecutionRequest) returns (PauseActivityExecutionResponse) { |
There was a problem hiding this comment.
Adding new requests
| temporal.api.activity.v1.ActivityOptions activity_options = 1; | ||
| } | ||
|
|
||
| // (-- api-linter: core::0134::request-mask-required=disabled |
There was a problem hiding this comment.
history service APIs routed on resource ID for all four operations
There was a problem hiding this comment.
I think we can have just one handler here in the activity lib that will handle both workflow and standalone activities.
There was a problem hiding this comment.
This comment was not addressed. You can call the old UpdateActivityOptions API that is already implemented from the activity handler.
| option (temporal.server.api.common.v1.api_category).category = API_CATEGORY_STANDARD; | ||
| } | ||
|
|
||
| // UpdateActivityExecutionOptions is called by the client to update the options of an activity |
There was a problem hiding this comment.
historyservice RPCs
| return &workflowservice.UnpauseWorkflowExecutionResponse{}, nil | ||
| } | ||
|
|
||
| func (wh *WorkflowHandler) PauseActivityExecution( |
There was a problem hiding this comment.
Stubs that will be implemented later
| ResetActivity(ctx context.Context, request *historyservice.ResetActivityRequest) (*historyservice.ResetActivityResponse, error) | ||
| PauseWorkflowExecution(ctx context.Context, request *historyservice.PauseWorkflowExecutionRequest) (*historyservice.PauseWorkflowExecutionResponse, error) | ||
| UnpauseWorkflowExecution(ctx context.Context, request *historyservice.UnpauseWorkflowExecutionRequest) (*historyservice.UnpauseWorkflowExecutionResponse, error) | ||
| UpdateActivityExecutionOptions(ctx context.Context, request *historyservice.UpdateActivityExecutionOptionsRequest) (*historyservice.UpdateActivityExecutionOptionsResponse, error) |
There was a problem hiding this comment.
Adding for code generation and mock generation
bergundy
left a comment
There was a problem hiding this comment.
I'm on the fence on whether workflow and standalone should have different handlers in the history service. In my mind they have a single handler in chasm/lib/activity/handler.go and that dispatches to the history handler. When we port workflow activities, the implementation of that handler would be very simple but for now you can inject a history handler into the activity handler and call the old methods from there.
You can see in my signal-with-start from workflow how to get a history handler injected.
| } | ||
|
|
||
| rpc PauseActivityExecution(PauseActivityExecutionRequest) returns (PauseActivityExecutionResponse) { | ||
| option (temporal.server.api.routing.v1.routing).business_id = "frontend_request.activity_id"; |
There was a problem hiding this comment.
What about when workflow_id is the business_id?
We can consider making the business_id a repeated field in the annotation and fallback from workflow_id to activity_id. See the protoc plugin that generates the code for how to achieve that.
There was a problem hiding this comment.
I don't think you can use resource ID here.
There was a problem hiding this comment.
Changing business_id to a repeated field is going to fire our breaking change checker, is this a breaking change we are OK with?
There was a problem hiding this comment.
I added a business_id_fallback which is not a breaking change, but if we wanted to add a repeated I can make that change.
There was a problem hiding this comment.
Changed back to a repeated business_id, does this need to be merged into main to prevent issues later on?
| temporal.api.activity.v1.ActivityOptions activity_options = 1; | ||
| } | ||
|
|
||
| // (-- api-linter: core::0134::request-mask-required=disabled |
There was a problem hiding this comment.
I think we can have just one handler here in the activity lib that will handle both workflow and standalone activities.
| // (-- api-linter: core::0134::request-mask-required=disabled | ||
| // (-- api-linter: core::0134::request-resource-required=disabled | ||
| message UpdateActivityExecutionOptionsRequest { | ||
| option (routing).workflow_id = "frontend_request.resource_id"; |
There was a problem hiding this comment.
Does this work? I think the resource ID has a prefix in these cases.
There was a problem hiding this comment.
Changed to workflow ID
| return &workflowservice.UnpauseWorkflowExecutionResponse{}, nil | ||
| } | ||
|
|
||
| func (wh *WorkflowHandler) PauseActivityExecution( |
There was a problem hiding this comment.
Put this in chasm/lib/activity/frontend.go as we have done for other activity APIs please.
dandavison
left a comment
There was a problem hiding this comment.
I took a stab at writing up my understanding of the PR. (Something like this could be useful as PR description if accurate).
(Just submitting one comment for now; got pulled away.)
--
The overall aim of this PR is to move towards supporting operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished according to whether or not the request has a workflow ID.
In fact, for Pause, Unpause, and Reset there is already a WA implementation with an experimental and deprecated public API. The API handlers added in this PR route to the current experimental implementation.
The end result is the following: we define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. Apart from UpdateOptions, the WA dipatch is to an existing implementation.
| ) (*workflowservice.PauseActivityExecutionResponse, error) { | ||
| if !h.config.Enabled(req.GetNamespace()) { | ||
| return nil, ErrStandaloneActivityDisabled | ||
| } |
There was a problem hiding this comment.
We're on the path for both SAA and WA here, so this means that WA Pause will fail if SAA is disabled. I think that's a bug.
There was a problem hiding this comment.
Fixed for all four
|
@dandavison I added your comment into the PR description and corrected some parts, for example updateoptions already exists in workflow activities |
| Reason: frontendReq.GetReason(), | ||
| Identity: frontendReq.GetIdentity(), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
How about in this PR, to prove some of the changes here are correct, you add new variants for the existing functional tests for all 4 operations that test them via the new API? AIUI that should work.
There was a problem hiding this comment.
I added a simple test for each path for success in the happy path (workflow) and failure on the SAA path. With each implementation it will require repeating the existing activity api suites for three scenarios:
- the existing activity operator API suite
- the new operator API targeted at workflow activity
- new operator API targeted at SAA
I omitted these to keep this PR easier to review for now but that will come in future PRs.
| @@ -457,6 +461,100 @@ func activityOptionsFromStartRequest(req *workflowservice.StartActivityExecution | |||
| } | |||
|
|
|||
| // applyActivityOptionsToStartRequest copies normalized values from ActivityOptions | |||
There was a problem hiding this comment.
This comment got left in the wrong place
There was a problem hiding this comment.
Add TODOs to validate all of the requests?
| } | ||
|
|
||
| func newHandler(config *Config, metricsHandler metrics.Handler, logger log.Logger, namespaceRegistry namespace.Registry) *handler { | ||
| func newHandler(config *Config, historyClient resource.HistoryClient, metricsHandler metrics.Handler, logger log.Logger, namespaceRegistry namespace.Registry) *handler { |
There was a problem hiding this comment.
You don't need a history client here, just get the grpc handler here and invoke it directly, skipping the network overhead. I did that in the signal-with-start from workflow WIP branch.
| temporal.api.activity.v1.ActivityOptions activity_options = 1; | ||
| } | ||
|
|
||
| // (-- api-linter: core::0134::request-mask-required=disabled |
There was a problem hiding this comment.
This comment was not addressed. You can call the old UpdateActivityOptions API that is already implemented from the activity handler.
There was a problem hiding this comment.
As I've mentioned before, no changes are needed in this service.
| # example: - temporal/server/api/.../message.proto | ||
| - temporal/server/api/persistence/v1/chasm.proto | ||
| - temporal/server/api/token/v1/message.proto | ||
| # TODO: Remove once this is stable. business_id was intentionally changed from |
There was a problem hiding this comment.
All of the exceptions here should be removed ASAP.
There was a problem hiding this comment.
This will have to stay until this PR is merged into main, then can be removed in a subsequent PR unless there's another method that I am missing
|
|
||
| // makeWorkflowFuncForNewAPITests returns a workflow function that runs a single activity with a | ||
| // long schedule-to-close timeout and the given retry policy. | ||
| func (s *standaloneActivityTestSuite) makeWorkflowFuncForNewAPITests(activityFn ActivityFunctions, retryPolicy *temporal.RetryPolicy) WorkflowFunction { |
There was a problem hiding this comment.
Don't put workflow tests in a file that is supposed to test standalone activities please. Also, I would wait with these tests until there's something implemented.
There was a problem hiding this comment.
@dandavison asked for them to validate the implementation.
There was a problem hiding this comment.
I do think it makes sense to add tests for the workflow activity portion, since that should be fully functioning. Instead of adding a new file tests/activity_api_execution_test.go is is possible to achieve that coverage by parameterizing the existing tests in activity_api_pause_test.go? I.e. confirming that all the existing test coverage for PauseActivityRequest still holds when using PauseActivityExecutionRequest.
I'm fine with adding stubbed tests for the new Standalone Activity functionality. It's healthy to confirm that the wiring in this PR works even though the full test can come later.
There was a problem hiding this comment.
I was able to parameterize the existing XXXActivity and XXXActivityExecution operators in the tests, looks cleaner and removes an extra test file
…rvice, separate wf and saa tests
|
|
||
| s.WaitForChannel(ctx, activityStartedCh) | ||
|
|
||
| _, err = s.FrontendClient().PauseActivity(ctx, &workflowservice.PauseActivityRequest{ |
There was a problem hiding this comment.
Should this call PauseActivityExecution instead?
| return nil, ErrStandaloneActivityDisabled | ||
| } | ||
|
|
||
| // TODO: validate request fields (e.g. namespace, identity length) |
There was a problem hiding this comment.
Let's add TODOs to validate that either workflowID or activityID is set before allowing it to be routed to a history shard using "".
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
* Add four new RPC methods to `historyservice`(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions) * Add new historyservice request/response messages wrapping api repos requests * Generated code First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities. The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) No runtime behavior changes.
What changed?
historyservice(PauseActivityExecution,UnpauseActivityExecution,ResetActivityExecution,UpdateACtivityExecutionOptions)Why?
First step towards supporting the four activity operator commands on both workflow activities and standalone CHASM activities.
The overall aim of this branch is to support operator API commands (Pause, Unpause, Reset, UpdateOptions) for both Standalone Activities (SAA) and Workflow Activities (WA). These will share the same new external workflowservice RPC entrypoints and will be distinguished by the presence of a workflow ID. Pause, Unpause, UpdateOptions, and Reset already exist for WA with an experimental and soon-to-be deprecated public API. The API handlers added in this PR route to the current experimental implementation. We define Frontend and History service RPC handlers in chasm/lib/activity. Requests for both SAA and WA are routed from Frontend to History using these handlers. The handler in the history service (part of the CHASM ActivityService which is hosted by History service) then inspects the workflow ID and activity ID and dispatches according to whether the request is for an SAA vs WA.
How did you test it?
Potential risks
No runtime behavior changes.