GetVersion implementation #182

yiminc-zz · 2017-07-11T00:49:48Z

This is based on #170, the only change is changing the map that attached to rootCtx from type map[string]*changeVersion to map[string]Version, as a result, also change the cadence.GetVersion() code a little bit.

coveralls · 2017-07-11T00:52:34Z

Coverage remained the same at 92.086% when pulling 16abadc on yiminc:get_version into 855bbc8 on uber-go:master.

molteanu · 2017-07-11T16:51:46Z

internal_event_handlers.go

-	if attributes.GetMarkerName() != sideEffectMarkerName {
+	switch attributes.GetMarkerName() {
+	case sideEffectMarkerName:
+		dec := gob.NewDecoder(bytes.NewBuffer(attributes.GetDetails()))


NIT: Could would be more readable if you extract the decoding into a helper method. (Same comment for decoding the "version marker"

Why are you not using encoding interface on hostEnv to decode, so we can switch easily with a different encoder.

sivakku · 2017-07-12T15:40:29Z

internal_decision_state_machine.go

+	markerID := fmt.Sprintf("%v_%v", versionMarkerName, changeID)
+	var buf bytes.Buffer
+	enc := gob.NewEncoder(&buf)
+	if err := enc.Encode(changeID); err != nil {


Why are you not going through encoding interface exposed through hostEnv.Encoder()? that would give us single place to switch different encoders.

sivakku · 2017-07-12T15:48:17Z

internal_event_handlers.go

+	if version > maxSupported {
+		panic(fmt.Sprintf("Workflow code is too old to support version %v "+
+			"for \"%v\" changeID. The maximum supported version is %v",
+			version, changeID, minSupported))


minSupported -> maxSupported

sivakku · 2017-07-12T15:50:10Z

internal_event_handlers.go

@@ -335,6 +335,40 @@ func (wc *workflowEnvironmentImpl) RequestCancelTimer(timerID string) {
 	wc.logger.Debug("RequestCancelTimer", zap.String(tagTimerID, timerID))
 }

+func validateVersion(changeID string, version, minSupported, maxSupported Version) {


Can these methods return error message and panic at GetVersion() level? sometimes seeing too many panics in library code gives a feeling that panic is accepted pattern but we only do it in certain method calls, where error is not obvious choice.

This is a helper method that has no much of logic inside. If we would return flag here and panic in GetVersion() level, then this method does not need to be exists.

you can return error instead of flag. it is still good to have validation as a separate method.

Use error when invalid version is expected in legit case, and call site can handle that error accordingly. In this case, invalid version is not expected, and it is not recoverable. So I think panic is appropriate here.

instead of panic'ing here, panic inside GetVersion() if validateVersion() returns error. so it is very clear that method panics.

sivakku · 2017-07-12T15:51:22Z

internal_event_handlers.go

+			version = DefaultVersion
+		}
+		validateVersion(changeID, version, minSupported, maxSupported)
+		wc.logger.Debug("GetVersion return version from a marker",


Can you hide this logger under the verbose check?

sivakku · 2017-07-12T15:51:54Z

internal_event_handlers.go

+	version = maxSupported
+	wc.decisionsHelper.recordVersionMarker(changeID, version)
+
+	wc.logger.Debug("GetVersion return",


same here? the more it looks this flag of checking to log looks hacky all over place.

sivakku · 2017-07-12T15:53:40Z

internal_event_handlers.go

-	if attributes.GetMarkerName() != sideEffectMarkerName {
+	switch attributes.GetMarkerName() {
+	case sideEffectMarkerName:
+		dec := gob.NewDecoder(bytes.NewBuffer(attributes.GetDetails()))


Why are you not using encoding interface on hostEnv to decode, so we can switch easily with a different encoder.

sivakku · 2017-07-12T15:58:33Z

internal_worker_base.go

@@ -53,6 +53,7 @@ type (
 		asyncActivityClient
 		workflowTimerClient
 		SideEffect(f func() ([]byte, error), callback resultHandler)
+		GetVersion(changeID string, maxSupported, minSupported Version) Version


The order of variables is inversed. minSupported, maxSupported. Can you add unit test for this layer as well?

sivakku · 2017-07-12T16:01:45Z

internal_workflow.go

@@ -327,6 +328,7 @@ func (d *syncWorkflowDefinition) Execute(env workflowEnvironment, input []byte)
 	d.rootCtx = WithValue(background, workflowEnvironmentContextKey, env)
 	var resultPtr *workflowResult
 	d.rootCtx = WithValue(d.rootCtx, workflowResultContextKey, &resultPtr)
+	d.rootCtx = WithValue(d.rootCtx, changeVersionsContextKey, make(map[string]Version))


What is the value if having this look up in this layer when you have the lookup already in task handler GetVersion()?

This layer server as a cache so we always return same version for same changeID, only if the changeID is known ID. The lookup at workflowEnvironment layer serve lookup from marker result. If a changeID is not present at workflowEnvironment layer, we would return DefaultVersion, but if a changeID is not present at here, we won't return DefaultVersion.

the 2 layer lookup can be combined, just updated.

coveralls · 2017-07-12T17:01:17Z

Coverage remained the same at 92.086% when pulling af4328c on yiminc:get_version into 855bbc8 on uber-go:master.

coveralls · 2017-07-12T17:01:17Z

Coverage remained the same at 92.086% when pulling af4328c on yiminc:get_version into 855bbc8 on uber-go:master.

coveralls · 2017-07-12T17:49:09Z

Coverage remained the same at 92.086% when pulling 4c809ef on yiminc:get_version into 855bbc8 on uber-go:master.

sivakku · 2017-07-12T22:05:36Z

internal_event_handlers.go

@@ -335,6 +335,40 @@ func (wc *workflowEnvironmentImpl) RequestCancelTimer(timerID string) {
 	wc.logger.Debug("RequestCancelTimer", zap.String(tagTimerID, timerID))
 }

+func validateVersion(changeID string, version, minSupported, maxSupported Version) {


you can return error instead of flag. it is still good to have validation as a separate method.

coveralls · 2017-07-12T22:37:14Z

Coverage remained the same at 92.086% when pulling 5edc877 on yiminc:get_version into 6efb266 on uber-go:master.

mfateev · 2017-07-12T22:38:16Z

internal_workflow_testsuite_test.go

+	workflowFn := func(ctx Context) error {
+		ctx = WithActivityOptions(ctx, s.activityOptions)
+		var f Future
+		v := GetVersion(ctx, "test_change_id", DefaultVersion, 2)


Looking at this we probably want to change DefaultVersion value to 1.

GetVersion implementation

16abadc

yiminc-zz requested review from mfateev and sivakku July 11, 2017 00:49

molteanu reviewed Jul 11, 2017

View reviewed changes

sivakku reviewed Jul 12, 2017

View reviewed changes

yiminc-zz added 2 commits July 12, 2017 10:03

address comments

047794b

combine 2 layer lookup into one

4c809ef

yiminc-zz force-pushed the get_version branch from af4328c to 4c809ef Compare July 12, 2017 17:45

sivakku approved these changes Jul 12, 2017

View reviewed changes

Merge branch 'master' into get_version

5edc877

yiminc-zz merged commit 2263e84 into uber-go:master Jul 12, 2017

mfateev reviewed Jul 12, 2017

View reviewed changes

yiminc-zz mentioned this pull request Jul 20, 2017

Added cadence.GetVersion #170

Closed

yiminc-zz deleted the get_version branch July 27, 2017 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetVersion implementation #182

GetVersion implementation #182

yiminc-zz commented Jul 11, 2017

coveralls commented Jul 11, 2017 •

edited

molteanu Jul 11, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

yiminc-zz Jul 12, 2017

sivakku Jul 12, 2017

yiminc-zz Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

sivakku Jul 12, 2017

yiminc-zz Jul 12, 2017

yiminc-zz Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017 •

edited

sivakku Jul 12, 2017

coveralls commented Jul 12, 2017 •

edited

mfateev Jul 12, 2017

GetVersion implementation #182

GetVersion implementation #182

Conversation

yiminc-zz commented Jul 11, 2017

coveralls commented Jul 11, 2017 • 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

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

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Jul 12, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Jul 11, 2017 •

edited

coveralls commented Jul 12, 2017 •

edited

coveralls commented Jul 12, 2017 •

edited