Conversation
alexshtin
left a comment
alexshtin
left a comment
There was a problem hiding this comment.
Looks good to me. Ready to approve after comments are addressed.
proto/internal/temporal/server/api/historyservice/v1/request_response.proto
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I always wanted to remove namespace_handler or at least change its name and interfaces. handler is suffix reserved for gRPC handlers, I wouldn't add new code to it but add it directly to workflow_handler.go.
There was a problem hiding this comment.
Here you have part of validation in workflow_handler.go and another part here. It adds more confusion than help.
There was a problem hiding this comment.
namespace_handle is a bad name, sure, and should be renamed.
But functions in namespace handle I added doesn't depend on gRPC.
So there is a clear separation - workflow_handle handle gRPC part, and "namespace...hm...handle" handles actual work.
All validation is a workflow_handle validation code done agains request.
| existingNamespace := getResponse.Namespace | ||
|
|
||
| config := existingNamespace.Config |
There was a problem hiding this comment.
I personally don't like such variables. getResponse (btw, it is a very bad name) is still accessible and someone will use one day. If you just rename getResponse to existing you will get the same English but w/o extra vars.
I agree, it is matter of personal preferences though.
There was a problem hiding this comment.
I think I copy/paster it from somewhere, getResponse was a weird name. I rename it.
But the rest - it is easier for me to read existingNamespace.Config then getNamespaceResponse.Namespace.Config, less cognitive load
ychebotarev
force-pushed
the
y_workflow_rules
branch
from
c744a72 to
2eab481
Compare
alexshtin
left a comment
alexshtin
left a comment
There was a problem hiding this comment.
I still don't agree with namespace_handler.go separation, but I won't fight for it.
ychebotarev
force-pushed
the
y_workflow_rules
branch
2 times, most recently
from
a9e0559 to
34bd801
Compare
ychebotarev
force-pushed
the
y_workflow_rules
branch
4 times, most recently
from
03f1aa8 to
42758bd
Compare
<!-- Describe what has changed in this PR --> **What changed?** Note: ->[Previous PR we use to review](https://github.com/temporalio/api/pull/550)<- It has lots of discussion, and current implementation is mostly what we settle on. With some changes from design review. Add definitions for the following API: * CreateWorkflowRule * DescribeWorkflowRule * DeleteWorkflowRule * ListWorkflowRules Add definitions for Rules, and Actions. Corresponding Server PR contains implementation of those APIs. <!-- Tell your future self why have you made these changes --> **Why?** Working on workflow rules. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** New API is added. <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** [Implementation](temporalio/temporal#7437)
<!-- Describe what has changed in this PR --> **What changed?** Note: ->[Previous PR we use to review](https://github.com/temporalio/api/pull/550)<- It has lots of discussion, and current implementation is mostly what we settle on. With some changes from design review. Add definitions for the following API: * CreateWorkflowRule * DescribeWorkflowRule * DeleteWorkflowRule * ListWorkflowRules Add definitions for Rules, and Actions. Corresponding Server PR contains implementation of those APIs. <!-- Tell your future self why have you made these changes --> **Why?** Working on workflow rules. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** New API is added. <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** [Implementation](temporalio/temporal#7437)
ychebotarev
force-pushed
the
y_workflow_rules
branch
from
e5af99c to
672d16e
Compare
2 participants
What changed?
Add implementation to the following API:
Add initial rule implementation - activity will be paused on activity Retry.
Add functional test for that.
All functions are behind feature flag.
Why?
Part of workflow rules work.
How did you test it?
Add unit test.
Add func test.
Potential risks
N/A