-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Standalone Activity List and Count ActivityExecutions #8789
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
Conversation
601fb05 to
65cb249
Compare
e349308 to
dbdead0
Compare
c06d36b to
155925e
Compare
chasm/lib/activity/activity.go
Outdated
| const ( | ||
| ActivityTypeSAAlias = "ActivityType" | ||
| ActivityStatusSAAlias = "ActivityStatus" | ||
| ActivityTaskQueueSAAlias = "ActivityTaskQueue" |
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.
| ActivityTaskQueueSAAlias = "ActivityTaskQueue" | |
| TaskQueueSAAlias = "TaskQueue" |
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.
Same here, it's just task queue.
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.
Discussed offline, we decided to keep this as-is for now. Other work will address.
chasm/lib/activity/activity.go
Outdated
| "google.golang.org/protobuf/types/known/durationpb" | ||
| "google.golang.org/protobuf/types/known/timestamppb" | ||
| ) | ||
|
|
||
| const ( | ||
| ActivityTypeSAAlias = "ActivityType" | ||
| ActivityStatusSAAlias = "ActivityStatus" |
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.
| ActivityStatusSAAlias = "ActivityStatus" | |
| ActivityStatusSAAlias = "ExecutionStatus" |
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.
Use the same non-qualified name as workflows.
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 I also ignore this comment and leave it as-is, as we have done for "ActivityTaskQueue"?
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've left this as "ActivityStatus" for now, pending advice (I know there were some discussions in progress relevant to this).
chasm/lib/activity/activity.go
Outdated
| ActivityTypeSearchAttribute = chasm.NewSearchAttributeKeyword(ActivityTypeSAAlias, chasm.SearchAttributeFieldKeyword01) | ||
| ActivityStatusSearchAttribute = chasm.NewSearchAttributeKeyword(ActivityStatusSAAlias, chasm.SearchAttributeFieldLowCardinalityKeyword01) | ||
| ActivityTaskQueueSearchAttribute = chasm.NewSearchAttributeKeyword(ActivityTaskQueueSAAlias, chasm.SearchAttributeFieldKeyword02) |
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 would be totally okay if we didn't stutter here and omitted the Activity prefix from these variables. We're already in the activity package.
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.
Done
chasm/lib/activity/frontend.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ctx = chasm.NewVisibilityManagerContext(ctx, h.visibilityManager) |
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 something that we expect to be done in an interceptor. Please coordinate with @awln-temporal @lina-temporal and @rodrigozhou on this.
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.
Is this blocking?
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.
OK I see the interceptor was added on main. I've rebased standalone-activity on main and rebased this PR on standalone-activity and got rid of this.
chasm/lib/activity/frontend.go
Outdated
| @@ -238,6 +325,21 @@ func (h *frontendHandler) validateAndPopulateStartRequest( | |||
| } | |||
| applyActivityOptionsToStartRequest(opts, req) | |||
|
|
|||
| // TODO: Unalias for validation, then restore aliased SA for CHASM visibility storage. The | |||
| // validator requires unaliased format but CHASM visibility expects aliased format. | |||
| originalSA := req.SearchAttributes | |||
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 would make validateAndNormalizeStartActivityExecutionRequest a method of the handler and unalias in there. Seems cleaner than mutating the request twice.
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.
Done.
| @@ -38,6 +38,12 @@ func ResolveSearchAttributeAlias( | |||
| return sadefs.WorkflowID, saType, nil | |||
| } | |||
|
|
|||
| // Handle ActivityId → WorkflowID transformation for standalone 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.
Visibility team should fix the need to hardcode here eventually. Add a TODO.
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.
Added TODO
| @@ -59,6 +59,8 @@ const ( | |||
| // any other custom search attribute. | |||
| ScheduleID = "ScheduleId" | |||
|
|
|||
| ActivityId = "ActivityId" | |||
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.
Same here, add a TODO to remove this hardcoded constant.
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.
Added TODO
tests/standalone_activity_test.go
Outdated
| customSAName := "ActivityCustomKeyword" | ||
| customSAValue := "custom-sa-test-value" | ||
|
|
||
| _, err := s.OperatorClient().AddSearchAttributes(ctx, &operatorservice.AddSearchAttributesRequest{ |
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 think there's a set of custom search attributes already defined for all functional tests.
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.
Thanks, using the predefined custom SAs
tests/standalone_activity_test.go
Outdated
| }) | ||
|
|
||
| t.Run("QueryByActivityStatus", func(t *testing.T) { | ||
| verifyListQuery(t, fmt.Sprintf("ActivityStatus = 'Scheduled' AND ActivityType = '%s'", activityType)) |
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 Scheduled? It's supposed Running as we have for ExecutionStatus for workflows (case sensitive?)
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.
Thanks, fixed
| s.Equal(activityID, exec.GetActivityId()) | ||
| s.Equal(runID, exec.GetRunId()) | ||
| s.Equal(activityType, exec.GetActivityType().GetName()) | ||
| s.Equal(enumspb.ACTIVITY_EXECUTION_STATUS_RUNNING, exec.GetStatus()) | ||
| s.NotNil(exec.GetScheduleTime()) |
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.
Make sure you cover all of the fields of ActivityExecutionListInfo in this test.
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.
All the fields are tested
083b015 to
4f365ba
Compare
e843c8e to
6f04ad6
Compare
afa1675 to
973dc71
Compare
21c8277 to
8f2aa3d
Compare
| } | ||
|
|
||
| // validateAndNormalizeStartActivityExecutionRequest validates and normalizes the standalone | ||
| // activity specific attributes. Note that this method mutates the input params; the caller must |
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.
nit: differentiate standalone activity "chasm" search attributes vs user defined custom search attributes.
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.
Done
|
|
||
| // Handle ActivityId → WorkflowID transformation for standalone activities. | ||
| // TODO: Remove this hardcoded transformation. | ||
| if name == sadefs.ActivityID { |
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.
Depending on which PR gets landed first, we'll need a follow up here to use the RegistrableComponent BusinessID option: #8803.
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.
Thanks, added the PR to the TODO comment.
| return nil | ||
| } | ||
|
|
||
| func validateAndNormalizeSearchAttributes( |
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 pattern will probably be common for most archetypes that support custom search attributes, might be worth moving to a common shared func in the chasm package, but not blocking.
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.
Thanks, I added a note
Implement standalone activity ListActivityExecutions and CountActivityExecutions.
Implement standalone activity ListActivityExecutions and CountActivityExecutions.
Note
Introduce List/Count ActivityExecutions for standalone activities with queryable search attributes and supporting validation/mapping.
ListActivityExecutionsandCountActivityExecutionsusingchasm.ListExecutions/CountExecutions; buildActivityExecutionListInfoand grouped counts; paging support.frontendHandler.validateAndPopulateStartRequestandvalidateAndNormalizeStartActivityExecutionRequest; add SA unaliasing/validation.ActivityType,ActivityStatus,ActivityTaskQueue) and implementVisibilitySearchAttributesProviderviaSearchAttributes.InternalStatusToAPIStatusand run-state mapping; use inbuildActivityExecutionInfo.library.go.ActivityId→WorkflowIdas a fallback in resolver; addActivityIDconstant insadefs/constants.go.Written by Cursor Bugbot for commit 74c4fa6. This will update automatically on new commits. Configure here.