Add Data Converter to support custom serialization/deserialization #463

vancexu · 2018-05-08T07:18:07Z

Users now can implement encoded.DataConverter and let Cadence use it to serialize/deserialize parameters that need to be sent over the wire.

To encode/decode workflow arguments, one should set DataConverter in 2 places:

Workflow worker, through worker.Options
Client, through client.Options

To encode/decode Activity/ChildWorkflow arguments, one should set DataConverter in 2 places:

Inside workflow code, use workflow.WithDataConverter to create new Context, and pass that context ExecuteActivity/ExecuteChildWorkflow calls. Cadence support using different data converter for different activity/childworkflow in same workflow.
Activity/Workflow worker that run these activity/childworkflow, through worker.Options.

yiminc-zz

You won't be able to specify different data converter for activity calls. Could you follow up with Max about this? I think that is something he wanted.

yiminc-zz · 2018-05-10T20:47:18Z

internal/activity.go

@@ -139,7 +140,7 @@ func RecordActivityHeartbeat(ctx context.Context, details ...interface{}) {
 	var err error
 	// We would like to be a able to pass in "nil" as part of details(that is no progress to report to)
 	if len(details) != 1 || details[0] != nil {
-		data, err = getHostEnvironment().encodeArgs(details)
+		data, err = encodeArgs(getDataConverterFromActivityCtx(ctx), details)


can we do getDataConverterFromActivityCtx(ctx).ToData(details...)? Make sure that getDataConverterFromActivityCtx() does not return nil. You can set the default data converter to the context if there is no customized one.

We could do that, but to follow the existing pattern, we only use encodeArgs encodeArg decodeArgs decodeArg with data converter passed in as first argument, to avoid directly use dc.ToData. This is safer because 1: you don't need to be careful about the array... 2: you won't panic if careless had dc been nil. So prefer to not change this

yiminc-zz · 2018-05-10T20:50:02Z

internal/client.go

@@ -335,11 +336,16 @@ func NewClient(service workflowserviceclient.Interface, domain string, options *
 		metricScope = options.MetricsScope
 	}
 	metricScope = tagScope(metricScope, tagDomain, domain)
+	dataConverter := newDefaultDataConverter()


try to avoid this allocation if a customized one is supplied.

yiminc-zz · 2018-05-11T16:58:56Z

internal/error_test.go

+		TimeoutType:      &timeoutType,
+	})
+	weh := &workflowExecutionEventHandlerImpl{context, nil}
+	weh.handleActivityTaskTimedOut(event)


can you use the test framework to cover this heartbeat case?

yiminc-zz · 2018-05-11T16:59:49Z

internal/internal_activity.go

@@ -68,6 +69,7 @@ type (
 		InputArgs                     []interface{}
 		ScheduleToCloseTimeoutSeconds int32
 		WorkflowInfo                  *WorkflowInfo
+		DataConverter                 encoded.DataConverter


you will need a rebase here, the executeLocalActivityParams has changed to contain localActivityOptions and other fields.

ok, thanks for remind here

yiminc-zz · 2018-05-11T17:04:28Z

internal/internal_activity.go

 	fType := reflect.TypeOf(f)
+	if dataConverter == nil {


can we make sure this dataConverter never be nil so we don't have to check this nil everywhere?

This is actually on purpose, for test code that use this, I want people just pass nil instead of figuration out how to deal with dc. So prefer not change

I mean our framework code to set a default value when user don't supply a custom one, that way, we guarantee the data converter won't be nil anywhere.

Understand, that's what I already did. But to be in defensive way, I feel it is still better to have this check, especially in this function.

yiminc-zz · 2018-05-11T22:15:47Z

internal/internal_worker.go

-func (th *hostEnvImpl) decodeArg(data []byte, to interface{}) error {
-	return th.decode(data, []interface{}{to})
+func decodeArg(dc encoded.DataConverter, data []byte, to interface{}) error {
+	if dc == nil {


could you have unit test to cover input with multiple parameters case

added TestDecodeArg in internal_worker_test

yiminc-zz · 2018-05-11T23:03:13Z

internal/workflow_testsuite.go

+	if !b.HasValues() {
+		return ErrNoData
+	}
+	for i, item := range b {


maybe do a sanity check to make sure len(valuePtr) <= len(b) and then loop through valuePtr instead of b.

coveralls · 2018-05-23T23:48:24Z

Coverage increased (+0.9%) to 66.397% when pulling a87be0f on dc2 into bbc380f on master.

yiminc-zz · 2018-05-24T17:16:55Z

encoded/encoded.go

@@ -38,4 +38,22 @@ type (
 		// Get extract the encoded values into strong typed value pointers.
 		Get(valuePtr ...interface{}) error
 	}
+
+	// DataConverter is used by the framework to serialize/deserialize method parameters that need to be sent over the wire.


to serialize/deserialize input and output of activity/workflow that ...

yiminc-zz · 2018-05-24T17:19:55Z

encoded/encoded.go

+	// To encode/decode workflow arguments, one should set DataConverter in two places:
+	//   1. Workflow worker, through worker.Options
+	//   2. Client, through client.Options
+	// To encode/decode Activity/ChildWorkflow arguments, one should set DataConverter two 2 places:


in two places

yiminc-zz · 2018-05-24T17:20:18Z

encoded/encoded.go

+	//   2. Client, through client.Options
+	// To encode/decode Activity/ChildWorkflow arguments, one should set DataConverter two 2 places:
+	//   1. Inside workflow code, use workflow.WithDataConverter to create new Context,
+	// and pass that context ExecuteActivity/ExecuteChildWorkflow calls.


pass that context to ...

yiminc-zz · 2018-05-24T17:46:21Z

internal/internal_activity.go

@@ -59,10 +60,12 @@ type (
 		HeartbeatTimeoutSeconds       int32
 		WaitForCancellation           bool
 		OriginalTaskListName          string
+		DataConverter                 encoded.DataConverter // used in testsuite


move this to executeActivityParams

yiminc-zz · 2018-05-24T17:46:36Z

internal/internal_activity.go

 	}

 	localActivityOptions struct {
 		ScheduleToCloseTimeoutSeconds int32
+		DataConverter                 encoded.DataConverter // used in testsuite


remove this one as it is already in executeLocalActivityParams

yiminc-zz · 2018-05-24T17:47:17Z

internal/internal_activity.go

@@ -186,6 +191,9 @@ func getValidatedLocalActivityOptions(ctx Context) (*localActivityOptions, error
 	if p.ScheduleToCloseTimeoutSeconds <= 0 {
 		return nil, errors.New("missing or negative ScheduleToCloseTimeoutSeconds")
 	}
+	if p.DataConverter == nil {


yiminc-zz · 2018-05-24T17:50:37Z

internal/internal_event_handlers.go

@@ -504,7 +511,7 @@ func (wc *workflowEnvironmentImpl) SideEffect(f func() ([]byte, error), callback
 			callback(result, err)
 			return
 		}
-		details, err = wc.hostEnv.encodeArgs([]interface{}{sideEffectID, result})
+		details, err = encodeArgs(wc.GetDataConverter(), []interface{}{sideEffectID, result})


you are using wc.dataConverter and wc.GetDataConverter(). Better make them consistent.

yes, changed all to use GetDataConverter()

yiminc-zz · 2018-05-24T17:56:38Z

internal/internal_worker.go

+	}
+
+	var encoder encoding
+	if isUseThriftEncoding(r) {


I think think this Thrift encoding is working. Could you file a task to make sure this thrift encoding works.

created #479

yiminc-zz · 2018-05-24T18:00:46Z

internal/internal_workflow_testsuite.go

@@ -282,6 +286,7 @@ func (env *testWorkflowEnvironmentImpl) newTestWorkflowEnvironmentForChild(param
 	childEnv.parentEnv = env
 	childEnv.startedHandler = startedHandler
 	childEnv.testWorkflowEnvironmentShared = env.testWorkflowEnvironmentShared
+	childEnv.workerOptions = WorkerOptions{DataConverter: params.dataConverter}


you need to copy workerOptions from parent to child, no just for DataConverter.

yiminc-zz · 2018-05-24T18:24:35Z

internal/internal_workflow_testsuite_test.go

@@ -392,6 +413,30 @@ func testWorkflowHello(ctx Context) (string, error) {
 	return result, nil
 }

+func testWorkflowHelloWithDataConverter(ctx Context) (string, error) {


can you move this function as workflowFn := func() ... into the Test_ActivityMockFunction_WithDataConverter

…ber-go#463) * Add data converter to support custom encode/decode * Add comments on DataConverter interface * Address comments * Address comments

vancexu requested review from mfateev and yiminc-zz May 8, 2018 07:18

vancexu mentioned this pull request May 8, 2018

Add Data Converter to support custom serialization/deserialization #442

Closed

yiminc-zz reviewed May 11, 2018

View reviewed changes

mfateev mentioned this pull request May 20, 2018

[Question] Workflow failure due to worker uber/cadence#751

Closed

Add data converter to support custom encode/decode

d4cacf3

vancexu force-pushed the dc2 branch from e441a55 to d4cacf3 Compare May 23, 2018 23:01

Add comments on DataConverter interface

ab6afd0

yiminc-zz approved these changes May 24, 2018

View reviewed changes

vancexu added 2 commits May 24, 2018 13:35

Address comments

ef08f09

Address comments

a87be0f

vancexu merged commit 7636327 into master May 24, 2018

vancexu deleted the dc2 branch May 24, 2018 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Data Converter to support custom serialization/deserialization #463

Add Data Converter to support custom serialization/deserialization #463

vancexu commented May 8, 2018 •

edited

yiminc-zz left a comment

yiminc-zz May 10, 2018

vancexu May 15, 2018

yiminc-zz May 10, 2018

vancexu May 15, 2018

yiminc-zz May 11, 2018

yiminc-zz May 11, 2018

vancexu May 15, 2018

yiminc-zz May 11, 2018

vancexu May 15, 2018

yiminc-zz May 16, 2018

vancexu May 16, 2018

yiminc-zz May 11, 2018

vancexu May 15, 2018 •

edited

yiminc-zz May 11, 2018

vancexu May 16, 2018

coveralls commented May 23, 2018 •

edited

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

vancexu May 24, 2018

yiminc-zz May 24, 2018

vancexu May 24, 2018

yiminc-zz May 24, 2018

yiminc-zz May 24, 2018

Add Data Converter to support custom serialization/deserialization #463

Add Data Converter to support custom serialization/deserialization #463

Conversation

vancexu commented May 8, 2018 • edited

yiminc-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu May 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu commented May 8, 2018 •

edited

vancexu May 15, 2018 •

edited

coveralls commented May 23, 2018 •

edited