Refactor and fix matching dynamic configs #857
Conversation
| @@ -236,6 +239,8 @@ const ( | |||
| DomainName | |||
| // TaskListName is the tasklist name | |||
| TaskListName | |||
| // TaskType is the task type (0:Decision, 1:Activity) | |||
| TaskType | |||
There was a problem hiding this comment.
Probably you need to have a filters item for TaskType filter.
There was a problem hiding this comment.
Good catch, addressed. Will also change and strengthen unit test for these later
service/matching/matchingEngine.go
Outdated
| @@ -230,7 +237,7 @@ func (e *matchingEngineImpl) AddActivityTask(addRequest *m.AddActivityTaskReques | |||
| sourceDomainID := addRequest.GetSourceDomainUUID() | |||
| taskListName := addRequest.TaskList.GetName() | |||
| taskListKind := common.TaskListKindPtr(addRequest.TaskList.GetKind()) | |||
| e.logger.Debugf("Received AddActivityTask for taskList=%v WorkflowID=%v, RunID=%v", | |||
| e.logger.Infof("Received AddActivityTask for taskList=%v WorkflowID=%v, RunID=%v", | |||
There was a problem hiding this comment.
This could get very noisy
service/matching/service.go
Outdated
| OutstandingTaskAppendsThreshold: dc.GetIntProperty(dynamicconfig.MatchingOutstandingTaskAppendsThreshold, 250), | ||
| MaxTaskBatchSize: dc.GetIntProperty(dynamicconfig.MatchingMaxTaskBatchSize, 100), | ||
| GetTasksBatchSize: dc.GetIntPropertyFilteredByTaskListInfo(dynamicconfig.MatchingGetTasksBatchSize, 1000), | ||
| UpdateAckInterval: dc.GetDurationPropertyFilteredByTaskListInfo(dynamicconfig.MatchingUpdateAckInterval, 10*time.Second), |
There was a problem hiding this comment.
Can you change this to 1 Minute. It could become a problem if number of tasklist grows in the system.
| } | ||
|
|
||
| func newTaskListConfig(id *taskListID, config *Config) *taskListConfig { | ||
| func newTaskListConfig(id *taskListID, config *Config, domainCache cache.DomainCache) (*taskListConfig, error) { | ||
| domainEntry, err := domainCache.GetDomainByID(id.domainID) |
There was a problem hiding this comment.
@wxing1292 Can you review this to make sure whether it is OK to host DomainCache within Matching Engine? Another option is to pass in DomainName along with DomainID on all Matching Engine API calls.
There was a problem hiding this comment.
@samarabbas the initialization in matching side looks fine
Previous matching configs are only filtered by tasklist, which is not correct because taskList might be same for different domain.
This change all matching configs to dynamic configs, and filtered by domain, taskListName, taskType.