Add Sticky Query to Cadence Server #464

wxing1292 · 2017-12-13T18:48:43Z

Add Sticky Query to Cadence Server
Add field in QueryWorkflowRequest, specifying whether to use sticky tasklist for query

This reverts commit 4176791.

…asklist for query

coveralls · 2017-12-13T19:13:14Z

Coverage increased (+0.2%) to 66.47% when pulling ec85132 on revert into 963d13c on master.

…se sticky query or not

yiminc-zz

you should not mix 2 changes into one PR. It also feels like an overkill to adding all those headers/versions just for query with no history.

yiminc-zz · 2017-12-14T18:11:40Z

.gen/go/shared/types.go

+	StickyScheduleToStartTimeoutSeconds *int32       `json:"stickyScheduleToStartTimeoutSeconds,omitempty"`
+	ClientLibraryVersion                *string      `json:"clientLibraryVersion,omitempty"`
+	ClientFeatureVersion                *string      `json:"clientFeatureVersion,omitempty"`
+	ClientLang                          *string      `json:"clientLang,omitempty"`


i'm not sure we want to public expose these to users.

removed

renamed getworkflow next event id to get mutable state, which returns these things.

yiminc-zz · 2017-12-14T18:13:44Z

common/client/clientFeature.go

+}
+
+// SupportStickyQuery whether a client support sticky query
+func (feature *FeatureImpl) SupportStickyQuery() bool {


you will need to check lang as well?

no, i do not need to check the lang,
but since i am adding this functionality, might as well also include this

yiminc-zz · 2017-12-14T18:17:24Z

service/history/mutableStateBuilder.go

+func (e *mutableStateBuilder) clearStickyness() {
+	e.executionInfo.StickyTaskList = ""
+	e.executionInfo.StickyScheduleToStartTimeout = 0
+	e.executionInfo.ClientLibraryVersion = ""


does not feel right to tie the client version to stickiness

why?

from the lease of sticky tasklist, all new worker will use sticky by default.
which means if server decide to put anything in the sticky tasklist, server need to be sure that the worker can handle it.

the ordinary tasklist is used when a worker first poll from server.
and at that time, server can only assume that worker has the very basic functionality.

samarabbas · 2017-12-15T00:51:43Z

common/client/clientFeature_test.go

+	featureVersion = ""
+	lang = ""
+	feature = NewFeatureImpl(libVersion, featureVersion, lang)
+	s.False(feature.SupportStickyQuery(), "Should support sticky query")


You may want to correct the message. "Not"

samarabbas · 2017-12-15T00:55:00Z

common/rpc.go

+	// FeatureVersionHeaderName refers to the name of the
+	// tchannel / http header that contains the client
+	// feature version
+	FeatureVersionHeaderName = "cadence-client-feature-version"


How does the client discover which feature version to pass in as header.

client change has already merged: cadence-workflow/cadence-go-client#316

What I meant is we need to document this feature version on server.

added:

// the feature version sent from client represents the
// feature set of the cadence client library support.
// This can be used for client capibility check, on
// Cadence server, for backward compatibility

// the feature version sent from client represents the
// version of the server feature set expected by the client.
// Cadence service is expected to reject clients that have incompatible major version.
// In some cases a client with a smaller major version than the service is allowed for the backwards compatibility. Clients with a larger major version are always rejected.

samarabbas · 2017-12-15T18:01:29Z

service/frontend/handler.go

-		DomainUUID:      common.StringPtr(taskToken.DomainID),
-		CompleteRequest: completeRequest,
-	})
+	var headers []yarpc.CallOption


Maybe you want to put this logic within the history/matching clients. I think we should just pass headers for all API calls on history and matching.

done for all matching and history client

samarabbas · 2017-12-15T18:30:36Z

service/frontend/handler.go

+
+		if len(persistenceToken) != 0 {
+			continuation, err = serializeHistoryToken(&getHistoryContinuationToken{
+				RunID:             *matchingResp.WorkflowExecution.RunId,


Use generated getter for RunId

samarabbas · 2017-12-15T19:09:19Z

service/matching/matchingEngine.go

@@ -275,9 +278,15 @@ pollLoop:
 				return emptyPollForDecisionTaskResponse, nil
 			}

+			isStickyEnabled := false
+			if describeResp.ExecutionConfiguration.StickyTaskList != nil && len(describeResp.ExecutionConfiguration.StickyTaskList.GetName()) != 0 {


I see this check in multiple places. Can we just have an API on ExecutionConfiguration for StickyTasklist?

samarabbas · 2017-12-15T19:31:02Z

service/frontend/handler.go

+		return nil, wh.error(err, scope)
+	}
+	clientFeature := client.NewFeatureImpl(
+		*response.ExecutionConfiguration.ClientLibraryVersion,


Can we use generated getter?

samarabbas · 2017-12-15T19:51:48Z

common/persistence/dataInterfaces.go

@@ -142,6 +142,9 @@ type (
 		CancelRequestID              string
 		StickyTaskList               string
 		StickyScheduleToStartTimeout int32
+		ClientLibraryVersion         string


You made the changes to mutable state API, but where are the cassandra persistence changes to support CRUD for these new values.

…ence of client version

samarabbas

Looks good. You can merge it in after addressing my minor comments.

samarabbas · 2017-12-15T22:15:14Z

client/history/client.go

@@ -460,3 +476,16 @@ redirectLoop:
 	}
 	return err
 }
+
+func aggregateYarpcOptions(ctx context.Context, opts ...yarpc.CallOption) []yarpc.CallOption {


This method is defined twice, can you move it to common?

samarabbas · 2017-12-15T22:17:12Z

common/rpc.go

+	// FeatureVersionHeaderName refers to the name of the
+	// tchannel / http header that contains the client
+	// feature version
+	FeatureVersionHeaderName = "cadence-client-feature-version"


What I meant is we need to document this feature version on server.

coveralls · 2017-12-16T00:07:56Z

Coverage increased (+0.4%) to 66.614% when pulling cdf0a36 on revert into bb26b98 on master.

yiminc-zz · 2018-01-09T19:59:14Z

service/frontend/handler.go

-		resp.StartedEventId = matchingResponse.StartedEventId
-		resp.Query = matchingResponse.Query
-		resp.BacklogCountHint = matchingResponse.BacklogCountHint
-		resp.Attempt = matchingResponse.Attempt


this Attempt should not be removed.

Wenquan Xing added 2 commits December 13, 2017 10:06

Revert "Revert "add support for sticky query (#452)" (#459)"

353a2f4

This reverts commit 4176791.

add field in QueryWorkflowRequest, specifying whether to use sticky t…

ec85132

…asklist for query

wxing1292 self-assigned this Dec 13, 2017

wxing1292 requested review from venkat1109, samarabbas and yiminc-zz December 13, 2017 18:48

use information from worker / client header to determine whether to u…

b4a14e9

…se sticky query or not

yiminc-zz reviewed Dec 14, 2017

View reviewed changes

samarabbas reviewed Dec 15, 2017

View reviewed changes

rename GetWorkflowNextEventID to GetMutableState, add missing persist…

c43a59d

…ence of client version

wxing1292 mentioned this pull request Dec 15, 2017

Implement the functionality to just wait for the last event when dumping history events #470

Merged

samarabbas approved these changes Dec 15, 2017

View reviewed changes

wxing1292 and others added 2 commits December 15, 2017 15:41

Merge branch 'master' into revert

05ca9c5

address comments

cdf0a36

wxing1292 force-pushed the revert branch from 8c644c0 to cdf0a36 Compare December 15, 2017 23:51

wxing1292 merged commit 4550c4e into master Dec 16, 2017

wxing1292 deleted the revert branch December 16, 2017 05:42

yiminc-zz reviewed Jan 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sticky Query to Cadence Server #464

Add Sticky Query to Cadence Server #464

wxing1292 commented Dec 13, 2017

coveralls commented Dec 13, 2017

yiminc-zz left a comment

yiminc-zz Dec 14, 2017

wxing1292 Dec 15, 2017

yiminc-zz Dec 14, 2017

wxing1292 Dec 14, 2017

yiminc-zz Dec 14, 2017

wxing1292 Dec 14, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

mfateev Dec 15, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas Dec 15, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas left a comment

samarabbas Dec 15, 2017

wxing1292 Dec 15, 2017

samarabbas Dec 15, 2017

coveralls commented Dec 16, 2017

yiminc-zz Jan 9, 2018

Add Sticky Query to Cadence Server #464

Add Sticky Query to Cadence Server #464

Conversation

wxing1292 commented Dec 13, 2017

coveralls commented Dec 13, 2017

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

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

samarabbas 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

coveralls commented Dec 16, 2017

Choose a reason for hiding this comment