improvement: bypass middle-man-struct for batch operations #8081

spkane31 · 2025-07-22T15:59:24Z

What changed?

Remove BatchParams to safely serialize proto -> json -> proto and add a BatchOperation proto definition

Why?

Remove custom serialization of proto for json safety.

How did you test it?

Potential risks

Could break batching backwards compatibility but tests should catch this

service/worker/batcher/workflow.go

…ts now

service/worker/batcher/fx.go

dnr

just a couple high-level comments

service/worker/batcher/activities.go

service/frontend/workflow_handler.go

service/worker/batcher/workflow.go

service/worker/batcher/activities.go

service/worker/batcher/fx.go

service/frontend/workflow_handler.go

…ral into spk/proto-serialization

proto/internal/temporal/server/api/batch/v1/request_response.proto

service/frontend/workflow_handler_test.go

service/worker/batcher/activities.go

service/worker/batcher/workflow.go

service/worker/batcher/activities.go

…ty query

spkane31 · 2025-08-01T21:29:50Z

service/frontend/workflow_handler.go

 	return &workflowservice.StartBatchOperationResponse{}, nil
 }

+func snakeCaseBatchType(batchType enumspb.BatchOperationType) string {


To maintain consistency with how we do these now a few of the batch operation types needed special casing

…serialization

…ral into spk/proto-serialization

…serialization

bergundy

Overall LGTM. Please look at the remaining comments before merging.

proto/internal/temporal/server/api/batch/v1/request_response.proto

service/worker/batcher/activities.go

service/frontend/workflow_handler.go

service/worker/batcher/activities.go

…serialization

service/worker/batcher/activities.go

dnr · 2025-08-04T21:52:12Z

service/frontend/workflow_handler.go

 		Query: fmt.Sprintf("%s = '%s' and %s = '%s'",
 			searchattribute.WorkflowType,
-			batcher.BatchWFTypeName,
+			batcher.BatchWFTypeProtobufName,


This may cause some confusion around the deployment transition. Is it worth listing both types temporarily?

Oh I missed this in the review, in person I requested to keep both. Addressed in #8144. We should merge that change before cutting a new server release. I would consider reverting this one until this fix is in.

service/worker/batcher/activities.go

dnr · 2025-08-04T21:59:28Z

service/worker/batcher/activities.go

+// nolint:revive,cognitive-complexity
+func (a *activities) BatchActivityWithProtobuf(ctx context.Context, batchParams *batchspb.BatchOperationInput) (HeartBeatDetails, error) {
+	logger := a.getActivityLogger(ctx)
+	hbd := HeartBeatDetails{}


is it worth considering moving heartbeat details to proto too? if we don't do it now we never can :)

dnr · 2025-08-04T22:00:40Z

service/worker/batcher/activities.go

+	rateLimiter := rate.NewLimiter(rateLimit, burstLimit)
+	taskCh := make(chan taskDetail, pageSize)
+	respCh := make(chan error, pageSize)
+	for i := 0; i < a.getOperationConcurrency(int(batchParams.Concurrency)); i++ {


Suggested change

for i := 0; i < a.getOperationConcurrency(int(batchParams.Concurrency)); i++ {

for range a.getOperationConcurrency(int(batchParams.Concurrency)) {

dnr · 2025-08-04T22:02:17Z

service/worker/batcher/activities.go

 							WorkflowExecution: &commonpb.WorkflowExecution{
-								WorkflowId: workflowID,
-								RunId:      runID,
+								WorkflowId: execution.WorkflowId,
+								RunId:      execution.RunId,
 							},


Suggested change

WorkflowExecution: &commonpb.WorkflowExecution{

WorkflowId: workflowID,

RunId: runID,

WorkflowId: execution.WorkflowId,

RunId: execution.RunId,

},

WorkflowExecution: execution,

dnr · 2025-08-04T22:02:26Z

service/worker/batcher/activities.go

 							WorkflowExecution: &commonpb.WorkflowExecution{
-								WorkflowId: workflowID,
-								RunId:      runID,
+								WorkflowId: execution.WorkflowId,
+								RunId:      execution.RunId,
 							},


Suggested change

WorkflowExecution: &commonpb.WorkflowExecution{

WorkflowId: workflowID,

RunId: runID,

WorkflowId: execution.WorkflowId,

RunId: execution.RunId,

},

WorkflowExecution: execution,

and same thing repeated several more times

dnr · 2025-08-04T22:03:54Z

service/worker/batcher/activities.go

+							WorkflowExecution: &commonpb.WorkflowExecution{
+								WorkflowId: execution.WorkflowId,
+								RunId:      execution.RunId,
+							},


Suggested change

WorkflowExecution: &commonpb.WorkflowExecution{

WorkflowId: execution.WorkflowId,

RunId: execution.RunId,

},

WorkflowExecution: execution,

and more below

## What changed? Change the backing query to avoid filtering on a specific workflow type since we now have two types for the transition period. ## Why? Fix for a bug introduced in #8081

…8081)" This reverts commit 356eeca.

This reverts the batch operation changes from PR 8081 (#8081) but only for files in the service/frontend directory, restoring the original BatchParams-based implementation.

This reverts the batch operation changes from PR 8081 (#8081) but only for files in the service/frontend directory, restoring the original BatchParams-based implementation. ## What changed? Revert #8081 frontend changes. ## Why? This change is not backwards compatible, in the future we can turn on the protobuf workflow ## How did you test it? - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Minimal, we're reverting the breaking change w/ this PR --------- Co-authored-by: Roey Berman <[email protected]>

improvement: bypass middle-man-struct for batch operations

91ec21f

dnr reviewed Jul 22, 2025

View reviewed changes

service/worker/batcher/workflow.go Outdated Show resolved Hide resolved

spkane31 added 3 commits July 22, 2025 14:37

migrated to using a protobuf for batch operations, need to update tes…

9448f39

…ts now

removing batchparams from last tests

52a30ca

merge conflicts

0aae0c8

spkane31 self-assigned this Jul 22, 2025

spkane31 mentioned this pull request Jul 22, 2025

Add a BatchOperations proto temporalio/api#620

Closed

spkane31 added 17 commits July 22, 2025 15:14

linter checks

44faa03

another merge conflict :(

c0d3a88

updating protos

9bb3411

hopefully fixing api issues

c2ce9ed

fixed bug setting operation

0698df7

linting skip and reduce function complexity of BatchActivity

f45ac22

nolint

5fd36d1

go.sum issue

d700916

fixing two unit tests

ce31cd3

patching identity through correctly

7c5b351

use payloads.Encode instead of sdk.ToPayloads

c1f3bc2

slices are set to nil by payloads.Encode, unlike with sdk.ToPayloads

5f4486e

fixing serialization for protobuf

9dee275

self review

80a5f9c

need backwards compatability

459ad84

workflow test duplication

5b29d78

duplicating workflow registration

9d4dfff

spkane31 commented Jul 23, 2025

View reviewed changes

service/worker/batcher/fx.go Outdated Show resolved Hide resolved

small diff fixes

0298808

dnr reviewed Jul 23, 2025

View reviewed changes

service/worker/batcher/activities.go Outdated Show resolved Hide resolved

service/frontend/workflow_handler.go Outdated Show resolved Hide resolved

bergundy reviewed Jul 23, 2025

View reviewed changes

got a batchworkflowexecutionrequest type but cannot find the method

1288ab4

spkane31 commented Jul 23, 2025

View reviewed changes

service/frontend/workflow_handler.go Outdated Show resolved Hide resolved

Merge branch 'spk/proto-serialization' of github.com:temporalio/tempo…

aebc4f9

…ral into spk/proto-serialization

bergundy reviewed Jul 30, 2025

View reviewed changes

spkane31 and others added 5 commits July 30, 2025 14:20

fixing namespace issues, renaming batchoperation -> batchoperationinput

5333cb7

fixing unit tests

b01bfc2

removing duplicated fields from proto definition, sanitizing visibili…

3ce004e

…ty query

escape entire search value

1feb76c

Merge branch 'main' into spk/proto-serialization

9c7dfaf

spkane31 commented Aug 1, 2025

View reviewed changes

spkane31 added 6 commits August 1, 2025 15:34

removing comments

508f259

Merge branch 'main' of github.com:temporalio/temporal into spk/proto-…

03b2812

…serialization

Merge branch 'spk/proto-serialization' of github.com:temporalio/tempo…

44ea522

…ral into spk/proto-serialization

fixing test

7e3a3f2

Merge branch 'main' of github.com:temporalio/temporal into spk/proto-…

fa9ad0a

…serialization

keep all fields in the proto

5fdb323

spkane31 requested a review from bergundy August 4, 2025 16:41

bergundy approved these changes Aug 4, 2025

View reviewed changes

spkane31 added 2 commits August 4, 2025 13:56

refactorings, bug fixes, removing rps

f45b1ae

Merge branch 'main' of github.com:temporalio/temporal into spk/proto-…

15a4428

…serialization

spkane31 merged commit 356eeca into main Aug 4, 2025
58 checks passed

spkane31 deleted the spk/proto-serialization branch August 4, 2025 20:26

dnr reviewed Aug 4, 2025

View reviewed changes

This was referenced Aug 5, 2025

Fix activity type escaping in batch query #8141

Closed

Fix ListBatchOperations list filter #8160

Merged

rodrigozhou added a commit that referenced this pull request Aug 11, 2025

Revert "improvement: bypass middle-man-struct for batch operations (#…

ed278db

…8081)" This reverts commit 356eeca.

rodrigozhou mentioned this pull request Aug 11, 2025

Revert "improvement: bypass middle-man-struct for batch operations (#8081)" #8184

Closed

5 tasks

spkane31 mentioned this pull request Aug 12, 2025

Revert PR 8081 changes in service/frontend directory only #8186

Merged

5 tasks

	for i := 0; i < a.getOperationConcurrency(int(batchParams.Concurrency)); i++ {
	for range a.getOperationConcurrency(int(batchParams.Concurrency)) {

improvement: bypass middle-man-struct for batch operations #8081

improvement: bypass middle-man-struct for batch operations #8081

Uh oh!

Conversation

spkane31 commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Why?

How did you test it?

Potential risks

Uh oh!

Uh oh!

Uh oh!

dnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergundy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spkane31 commented Jul 22, 2025 •

edited

Loading