Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dynamic config for EnableReadVisibilityFromOS #6560

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+23 −4
Open

Add dynamic config for EnableReadVisibilityFromOS #6560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions common/dynamicconfig/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,6 +1520,12 @@ const (
// Default value: true
// Allowed filters: DomainName
EnableReadVisibilityFromPinot
// EnableReadVisibilityFromOS is key for enable read from opensearch or db visibility, usually using with AdvancedVisibilityWritingMode for seamless migration from db visibility to advanced visibility
// KeyName: system.enableReadVisibilityFromOS
// Value type: Bool
// Default value: true
// Allowed filters: DomainName
EnableReadVisibilityFromOS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 bool flags which can be set independently. What if more than one is true or all is false? Behavior is not clear from config perspective. Ideally there should be one flag ReadVisibilityStoreName

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that can help preventing adding more flags here. When advanced visibility is enabled, it will first read from the advanced visibility store (ES or OS/Pinot), ES always has the most priority. If all of them are false, it will fall back to read from DB.
https://github.com/cadence-workflow/cadence/blob/master/common/persistence/visibility_dual_manager.go#L318

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/cadence-workflow/cadence/blob/master/common/persistence/visibility_triple_manager.go#L666
the triple manager has more cases

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new dynamicconfig to all the environments is also a lot of manual work. If it's feasible to switch to a single flag let's do that

// EnableVisibilityDoubleRead is the key for enable double read for a latency comparison
// KeyName: system.EnableVisibilityDoubleRead
// Value type: Bool
Expand Down Expand Up @@ -3919,6 +3925,12 @@ var BoolKeys = map[BoolKey]DynamicBool{
Description: "EnableReadVisibilityFromPinot is key for enable read from pinot or db visibility, usually using with AdvancedVisibilityWritingMode for seamless migration from db visibility to advanced visibility",
DefaultValue: true,
},
EnableReadVisibilityFromOS: {
KeyName: "system.enableReadVisibilityFromOS",
Filters: []Filter{DomainName},
Description: "EnableReadVisibilityFromOS is key for enable read from OpenSearch or db visibility, usually using with AdvancedVisibilityWritingMode for seamless migration from db visibility to advanced visibility",
DefaultValue: true,
},
EnableVisibilityDoubleRead: {
KeyName: "system.enableVisibilityDoubleRead",
Filters: []Filter{DomainName},
Expand Down
4 changes: 2 additions & 2 deletions common/persistence/client/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func (f *factoryImpl) NewVisibilityManager(
visibilityFromOS,
visibilityFromES,
resourceConfig.EnableReadVisibilityFromES, // Didn't add new config for EnableReadVisibilityFromOS since we will use es-visibility and version: "os2" when migration is done
resourceConfig.EnableReadVisibilityFromES, // this controls read from source(ES), will be the primary read source
resourceConfig.EnableReadVisibilityFromOS, // this controls read from source(ES), will be the primary read source
resourceConfig.AdvancedVisibilityMigrationWritingMode,
resourceConfig.EnableLogCustomerQueryParameter,
resourceConfig.EnableVisibilityDoubleRead,
Expand All @@ -343,7 +343,7 @@ func (f *factoryImpl) NewVisibilityManager(
return p.NewVisibilityDualManager(
visibilityFromDB,
visibilityFromOS,
resourceConfig.EnableReadVisibilityFromES, //Didn't add new config for EnableReadVisibilityFromOS since we will use es-visibility and version: "os2" when migration is done
resourceConfig.EnableReadVisibilityFromOS, //Didn't add new config for EnableReadVisibilityFromOS since we will use es-visibility and version: "os2" when migration is done
resourceConfig.AdvancedVisibilityWritingMode,
f.logger,
), nil
Expand Down
2 changes: 2 additions & 0 deletions common/service/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ type (
AdvancedVisibilityMigrationWritingMode dynamicconfig.StringPropertyFn
// EnableReadVisibilityFromPinot is the read mode of visibility
EnableReadVisibilityFromPinot dynamicconfig.BoolPropertyFnWithDomainFilter
// EnableReadVisibilityFromOS is the read mode of visibility
EnableReadVisibilityFromOS dynamicconfig.BoolPropertyFnWithDomainFilter
// EnableVisibilityDoubleRead is to enable double read for a latency comparison
EnableVisibilityDoubleRead dynamicconfig.BoolPropertyFnWithDomainFilter
// EnableLogCustomerQueryParameter is to enable log customer parameters
Expand Down
4 changes: 2 additions & 2 deletions service/frontend/api/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3202,8 +3202,8 @@ func (wh *WorkflowHandler) convertIndexedKeyToThrift(keys map[string]interface{}
}

func (wh *WorkflowHandler) isListRequestPageSizeTooLarge(pageSize int32, domain string) bool {
return common.IsAdvancedVisibilityReadingEnabled(wh.config.EnableReadVisibilityFromES(domain), wh.config.IsAdvancedVisConfigExist) &&
pageSize > int32(wh.config.ESIndexMaxResultWindow())
return common.IsAdvancedVisibilityReadingEnabled(wh.config.EnableReadVisibilityFromES(domain) || wh.config.EnableReadVisibilityFromOS(domain) || wh.config.EnableReadVisibilityFromPinot(domain),
wh.config.IsAdvancedVisConfigExist) && pageSize > int32(wh.config.ESIndexMaxResultWindow())
}

// GetClusterInfo return information about cadence deployment
Expand Down
2 changes: 2 additions & 0 deletions service/frontend/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type Config struct {
VisibilityListMaxQPS dynamicconfig.IntPropertyFnWithDomainFilter
EnableReadVisibilityFromES dynamicconfig.BoolPropertyFnWithDomainFilter
EnableReadVisibilityFromPinot dynamicconfig.BoolPropertyFnWithDomainFilter
EnableReadVisibilityFromOS dynamicconfig.BoolPropertyFnWithDomainFilter
EnableVisibilityDoubleRead dynamicconfig.BoolPropertyFnWithDomainFilter
EnableLogCustomerQueryParameter dynamicconfig.BoolPropertyFnWithDomainFilter
// deprecated: never read from
Expand Down Expand Up @@ -134,6 +135,7 @@ func NewConfig(dc *dynamicconfig.Collection, numHistoryShards int, isAdvancedVis
ESVisibilityListMaxQPS: dc.GetIntPropertyFilteredByDomain(dynamicconfig.FrontendESVisibilityListMaxQPS),
EnableReadVisibilityFromES: dc.GetBoolPropertyFilteredByDomain(dynamicconfig.EnableReadVisibilityFromES),
EnableReadVisibilityFromPinot: dc.GetBoolPropertyFilteredByDomain(dynamicconfig.EnableReadVisibilityFromPinot),
EnableReadVisibilityFromOS: dc.GetBoolPropertyFilteredByDomain(dynamicconfig.EnableReadVisibilityFromOS),
EnableLogCustomerQueryParameter: dc.GetBoolPropertyFilteredByDomain(dynamicconfig.EnableLogCustomerQueryParameter),
EnableVisibilityDoubleRead: dc.GetBoolPropertyFilteredByDomain(dynamicconfig.EnableVisibilityDoubleRead),
ESIndexMaxResultWindow: dc.GetIntProperty(dynamicconfig.FrontendESIndexMaxResultWindow),
Expand Down
1 change: 1 addition & 0 deletions service/frontend/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ func TestNewConfig(t *testing.T) {
"ESVisibilityListMaxQPS": {dynamicconfig.FrontendESVisibilityListMaxQPS, 5},
"EnableReadVisibilityFromES": {dynamicconfig.EnableReadVisibilityFromES, true},
"EnableReadVisibilityFromPinot": {dynamicconfig.EnableReadVisibilityFromPinot, false},
"EnableReadVisibilityFromOS": {dynamicconfig.EnableReadVisibilityFromOS, false},
"EnableLogCustomerQueryParameter": {dynamicconfig.EnableLogCustomerQueryParameter, true},
"EnableVisibilityDoubleRead": {dynamicconfig.EnableVisibilityDoubleRead, false},
"ESIndexMaxResultWindow": {dynamicconfig.FrontendESIndexMaxResultWindow, 6},
Expand Down
1 change: 1 addition & 0 deletions service/frontend/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func NewService(
EnableReadVisibilityFromES: serviceConfig.EnableReadVisibilityFromES,
AdvancedVisibilityWritingMode: nil, // frontend service never write
EnableReadVisibilityFromPinot: serviceConfig.EnableReadVisibilityFromPinot,
EnableReadVisibilityFromOS: serviceConfig.EnableReadVisibilityFromOS,
EnableLogCustomerQueryParameter: serviceConfig.EnableLogCustomerQueryParameter,
EnableVisibilityDoubleRead: serviceConfig.EnableVisibilityDoubleRead,

Expand Down
1 change: 1 addition & 0 deletions service/history/resource/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ func New(
AdvancedVisibilityWritingMode: config.AdvancedVisibilityWritingMode,
AdvancedVisibilityMigrationWritingMode: config.AdvancedVisibilityMigrationWritingMode,
EnableReadVisibilityFromPinot: nil, // history service never read,
EnableReadVisibilityFromOS: nil, // history service never read,
EnableLogCustomerQueryParameter: nil, // log customer parameter will be done in front-end

EnableDBVisibilitySampling: config.EnableVisibilitySampling,
Expand Down
Loading