From cc5085e3c82a5ce1f4c2b16a10dda9df6661ba50 Mon Sep 17 00:00:00 2001 From: Bowen Xiao Date: Fri, 9 Aug 2024 10:49:34 -0700 Subject: [PATCH 1/6] Bug fix: isCron return error --- common/definition/indexedKeys.go | 5 +++++ common/pinot/pinotQueryValidator.go | 23 +++++++++++++++++++++++ common/pinot/pinotQueryValidator_test.go | 17 +++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/common/definition/indexedKeys.go b/common/definition/indexedKeys.go index d9f993fb9d0..bcdce1620f2 100644 --- a/common/definition/indexedKeys.go +++ b/common/definition/indexedKeys.go @@ -110,3 +110,8 @@ func IsSystemIndexedKey(key string) bool { _, ok := systemIndexedKeys[key] return ok } + +// IsSystemBoolKey return true is key is system added bool key +func IsSystemBoolKey(key string) bool { + return systemIndexedKeys[key] == types.IndexedValueTypeBool +} diff --git a/common/pinot/pinotQueryValidator.go b/common/pinot/pinotQueryValidator.go index abfc270d40e..dc04cbcd82a 100644 --- a/common/pinot/pinotQueryValidator.go +++ b/common/pinot/pinotQueryValidator.go @@ -232,6 +232,24 @@ func (qv *VisibilityQueryValidator) IsValidSearchAttributes(key string) bool { return isValidKey } +func (qv *VisibilityQueryValidator) processSystemBoolKey(colNameStr string, comparisonExpr sqlparser.ComparisonExpr) (string, error) { + // case1: isCron = false + colVal, ok := comparisonExpr.Right.(sqlparser.BoolVal) + if !ok { + // case2: isCron = "false" or isCron = 'false' + sqlVal, ok := comparisonExpr.Right.(*sqlparser.SQLVal) + if !ok { + return "", fmt.Errorf("failed to process a bool key to SQLVal: %v", comparisonExpr.Right) + } + colValStr := string(sqlVal.Val) + if strings.ToLower(colValStr) != "false" && strings.ToLower(colValStr) != "true" { + return "", fmt.Errorf("invalid bool value in pinot_query_validator: %s", colValStr) + } + return fmt.Sprintf("%s = %s", colNameStr, colValStr), nil + } + return fmt.Sprintf("%s = %v", colNameStr, colVal), nil +} + func (qv *VisibilityQueryValidator) processSystemKey(expr sqlparser.Expr) (string, error) { comparisonExpr := expr.(*sqlparser.ComparisonExpr) buf := sqlparser.NewTrackedBuffer(nil) @@ -242,6 +260,11 @@ func (qv *VisibilityQueryValidator) processSystemKey(expr sqlparser.Expr) (strin } colNameStr := colName.Name.String() + // handle system bool key + if definition.IsSystemBoolKey(colNameStr) { + return qv.processSystemBoolKey(colNameStr, *comparisonExpr) + } + if comparisonExpr.Operator == sqlparser.LikeStr { colVal, ok := comparisonExpr.Right.(*sqlparser.SQLVal) if !ok { diff --git a/common/pinot/pinotQueryValidator_test.go b/common/pinot/pinotQueryValidator_test.go index 9f63181095a..7b50d0be9bf 100644 --- a/common/pinot/pinotQueryValidator_test.go +++ b/common/pinot/pinotQueryValidator_test.go @@ -279,6 +279,23 @@ func TestValidateQuery(t *testing.T) { validated: "", err: "invalid IN expression, value", }, + "case21-1: test bool value- system key- no quotes": { + query: "IsCron = true", + validated: "IsCron = true", + }, + "case21-2: test bool value- system key- single quotes": { + query: "IsCron = 'true'", + validated: "IsCron = true", + }, + "case21-3: test bool value- system key- double quotes": { + query: "IsCron = \"true\"", + validated: "IsCron = true", + }, + "case21-4: test bool value- system key- invalid value": { + query: "IsCron = 1", + validated: "", + err: "invalid bool value in pinot_query_validator: 1", + }, } for name, test := range tests { From 710213322ecdd486a2106febbe43815aa6cc1e6b Mon Sep 17 00:00:00 2001 From: Bowen Xiao Date: Tue, 10 Sep 2024 16:55:59 -0700 Subject: [PATCH 2/6] add log for jitter start time --- common/util.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/common/util.go b/common/util.go index c406d51c00a..937fdb69a6a 100644 --- a/common/util.go +++ b/common/util.go @@ -25,6 +25,7 @@ import ( "encoding/json" "errors" "fmt" + "go.uber.org/zap" "math" "math/rand" "sort" @@ -579,6 +580,17 @@ func CreateHistoryStartWorkflowRequest( } else if jitterStartSeconds > 0 { // Add a random jitter to start time, if requested. firstDecisionTaskBackoffSeconds += rand.Int31n(jitterStartSeconds + 1) + + // need a logger to log firstDecisionTaskBackoffSeconds and jitter start time for debug purpose + // will remove after the debug + // don't check err because don't want to break the function + logger, _ := zap.NewProduction() + if startRequest.Domain == "cadence-canary" { + logger.Info("Canary jitter start time debug log. ", + zap.Int32("jitterStartSeconds", jitterStartSeconds), + zap.Int32("firstDecisionTaskBackoffSeconds", firstDecisionTaskBackoffSeconds), + ) + } } } From 11eff7a53d21ac403a8fb5c37dc2d98cb3306715 Mon Sep 17 00:00:00 2001 From: Bowen Xiao Date: Tue, 10 Sep 2024 18:32:43 -0700 Subject: [PATCH 3/6] add log outside of CreateHistoryStartWorkflowRequest function --- common/util.go | 12 ------------ service/frontend/api/handler.go | 15 ++++++++++++++- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/common/util.go b/common/util.go index 937fdb69a6a..c406d51c00a 100644 --- a/common/util.go +++ b/common/util.go @@ -25,7 +25,6 @@ import ( "encoding/json" "errors" "fmt" - "go.uber.org/zap" "math" "math/rand" "sort" @@ -580,17 +579,6 @@ func CreateHistoryStartWorkflowRequest( } else if jitterStartSeconds > 0 { // Add a random jitter to start time, if requested. firstDecisionTaskBackoffSeconds += rand.Int31n(jitterStartSeconds + 1) - - // need a logger to log firstDecisionTaskBackoffSeconds and jitter start time for debug purpose - // will remove after the debug - // don't check err because don't want to break the function - logger, _ := zap.NewProduction() - if startRequest.Domain == "cadence-canary" { - logger.Info("Canary jitter start time debug log. ", - zap.Int32("jitterStartSeconds", jitterStartSeconds), - zap.Int32("firstDecisionTaskBackoffSeconds", firstDecisionTaskBackoffSeconds), - ) - } } } diff --git a/service/frontend/api/handler.go b/service/frontend/api/handler.go index 95b1885f55c..4d262e978bb 100644 --- a/service/frontend/api/handler.go +++ b/service/frontend/api/handler.go @@ -1889,13 +1889,26 @@ func (wh *WorkflowHandler) StartWorkflowExecution( if err != nil { return nil, err } - wh.GetLogger().Debug("Start workflow execution request domainID", tag.WorkflowDomainID(domainID)) historyRequest, err := common.CreateHistoryStartWorkflowRequest( domainID, startRequest, time.Now(), wh.getPartitionConfig(ctx, domainName)) if err != nil { return nil, err } + // for debugging jitter workflow + // will be removed later + jitterStartSeconds := startRequest.GetJitterStartSeconds() + if historyRequest.StartRequest.Domain == "cadence-canary" && jitterStartSeconds > 0 { + wh.GetLogger().Debug("Start workflow execution request domainID", + tag.WorkflowDomainID(domainID), + tag.WorkflowID(startRequest.WorkflowID), + tag.Dynamic("JitterStartSeconds", jitterStartSeconds), + tag.Dynamic("firstDecisionTaskBackoffSeconds", historyRequest.GetFirstDecisionTaskBackoffSeconds()), + ) + } else { + wh.GetLogger().Debug("Start workflow execution request domainID", tag.WorkflowDomainID(domainID)) + } + resp, err = wh.GetHistoryClient().StartWorkflowExecution(ctx, historyRequest) if err != nil { return nil, err From 410532a9396de964cd8a5cd659ba792bcb545be4 Mon Sep 17 00:00:00 2001 From: Bowen Xiao Date: Tue, 10 Sep 2024 18:43:33 -0700 Subject: [PATCH 4/6] refactor --- service/frontend/api/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/frontend/api/handler.go b/service/frontend/api/handler.go index 4d262e978bb..48473d48e83 100644 --- a/service/frontend/api/handler.go +++ b/service/frontend/api/handler.go @@ -1898,7 +1898,7 @@ func (wh *WorkflowHandler) StartWorkflowExecution( // for debugging jitter workflow // will be removed later jitterStartSeconds := startRequest.GetJitterStartSeconds() - if historyRequest.StartRequest.Domain == "cadence-canary" && jitterStartSeconds > 0 { + if startRequest.Domain == "cadence-canary" && jitterStartSeconds > 0 { wh.GetLogger().Debug("Start workflow execution request domainID", tag.WorkflowDomainID(domainID), tag.WorkflowID(startRequest.WorkflowID), From 0dcbc0c686f60af822d6ad1199af9a33a2f6b042 Mon Sep 17 00:00:00 2001 From: Bowen Xiao Date: Tue, 10 Sep 2024 18:44:30 -0700 Subject: [PATCH 5/6] refactor --- service/frontend/api/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/frontend/api/handler.go b/service/frontend/api/handler.go index 48473d48e83..629c1dbf1a0 100644 --- a/service/frontend/api/handler.go +++ b/service/frontend/api/handler.go @@ -1898,7 +1898,7 @@ func (wh *WorkflowHandler) StartWorkflowExecution( // for debugging jitter workflow // will be removed later jitterStartSeconds := startRequest.GetJitterStartSeconds() - if startRequest.Domain == "cadence-canary" && jitterStartSeconds > 0 { + if startRequest.GetDomain() == "cadence-canary" && jitterStartSeconds > 0 { wh.GetLogger().Debug("Start workflow execution request domainID", tag.WorkflowDomainID(domainID), tag.WorkflowID(startRequest.WorkflowID), From 78284c949113027ee026ad0405962a782aa3c4a7 Mon Sep 17 00:00:00 2001 From: Bowen Xiao Date: Wed, 11 Sep 2024 14:17:17 -0700 Subject: [PATCH 6/6] add unit test --- service/frontend/api/handler_test.go | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/service/frontend/api/handler_test.go b/service/frontend/api/handler_test.go index 01cad1d331a..cb8affdf408 100644 --- a/service/frontend/api/handler_test.go +++ b/service/frontend/api/handler_test.go @@ -61,6 +61,7 @@ import ( const ( numHistoryShards = 10 testDomain = "test-domain" + canaryDomain = "cadence-canary" testDomainID = "e4f90ec0-1313-45be-9877-8aa41f72a45a" testWorkflowID = "test-workflow-id" testRunID = "2c8b555f-1f55-4955-9d1c-b980194555c9" @@ -596,6 +597,39 @@ func (s *workflowHandlerSuite) TestStartWorkflowExecution_IsolationGroupDrained( s.IsType(err, &types.BadRequestError{}) } +func (s *workflowHandlerSuite) TestStartWorkflowExecution_LogJitterTime() { + config := s.newConfig(dc.NewInMemoryClient()) + config.UserRPS = dc.GetIntPropertyFn(10) + wh := s.getWorkflowHandler(config) + jitterStart := int32(10) + + startWorkflowExecutionRequest := &types.StartWorkflowExecutionRequest{ + Domain: canaryDomain, + WorkflowID: "workflow-id", + WorkflowType: &types.WorkflowType{ + Name: "workflow-type", + }, + TaskList: &types.TaskList{ + Name: "task-list", + }, + JitterStartSeconds: &jitterStart, + ExecutionStartToCloseTimeoutSeconds: common.Int32Ptr(1), + TaskStartToCloseTimeoutSeconds: common.Int32Ptr(1), + RetryPolicy: &types.RetryPolicy{ + InitialIntervalInSeconds: 1, + BackoffCoefficient: 2, + MaximumIntervalInSeconds: 2, + MaximumAttempts: 1, + ExpirationIntervalInSeconds: 1, + }, + RequestID: uuid.New(), + } + s.mockDomainCache.EXPECT().GetDomainID(canaryDomain).Return(s.testDomainID, nil).Times(2) + s.mockHistoryClient.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any()).Return(&types.StartWorkflowExecutionResponse{RunID: "test-rid"}, nil) + _, err := wh.StartWorkflowExecution(context.Background(), startWorkflowExecutionRequest) + s.NoError(err) +} + func (s *workflowHandlerSuite) TestDiagnoseWorkflowExecution_Success() { wh := s.getWorkflowHandler(s.newConfig(dc.NewInMemoryClient()))