Support visibility dual write to different ES indices #2359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexshtin
reviewed
Jan 11, 2022
Comment on lines
610
to
618
| if overStartTime { | ||
| params.Sorter = append(params.Sorter, elastic.NewFieldSort(searchattribute.StartTime).Desc()) | ||
| } else { | ||
| params.Sorter = append(params.Sorter, elastic.NewFieldSort(searchattribute.CloseTime).Desc()) | ||
| } | ||
|
|
||
| // RunID is explicit tiebreaker. | ||
| params.Sorter = append(params.Sorter, elastic.NewFieldSort(searchattribute.RunID).Desc()) | ||
| // RunID is explicit tiebreaker. | ||
| params.Sorter = append(params.Sorter, elastic.NewFieldSort(searchattribute.RunID).Desc()) | ||
| } |
There was a problem hiding this comment.
Please add TODO to remove this else block together with v2IndexPrefix const as soon as new approach is proven to work.
There was a problem hiding this comment.
Changed to use the default sorting order in all cases.
| @@ -138,7 +179,7 @@ func NewStandardManager( | |||
| } | |||
|
|
|||
| func NewAdvancedManager( | |||
| defaultIndexName string, | |||
| indexName string, | |||
There was a problem hiding this comment.
The reason is was called defaultIndexName because there is partial support for index per namespace. Metadata has indexName field which is not propagated and used yet. The one from config supposed to be default index name if it is not configured on namespace level. I think you should rename it back.
common/dynamicconfig/constants.go
Outdated
| @@ -70,7 +70,9 @@ var Keys = map[Key]string{ | |||
| AdvancedVisibilityPersistenceMaxReadQPS: "system.advancedVisibilityPersistenceMaxReadQPS", | |||
| AdvancedVisibilityPersistenceMaxWriteQPS: "system.advancedVisibilityPersistenceMaxWriteQPS", | |||
| AdvancedVisibilityWritingMode: "system.advancedVisibilityWritingMode", | |||
| EnableWriteToSecondaryVisibility: "system.enableWriteToSecondaryVisibility", | |||
There was a problem hiding this comment.
may be:
Suggested change
| EnableWriteToSecondaryVisibility: "system.enableWriteToSecondaryVisibility", | |
| EnableWriteToSecondaryAdvancedVisibility: "system.enableWriteToSecondaryAdvancedVisibility", |
| @@ -44,7 +44,7 @@ func NewManager( | |||
| persistenceCfg config.Persistence, | |||
| persistenceResolver resolver.ServiceResolver, | |||
|
|
|||
| defaultIndexName string, | |||
| esConfig *esclient.Config, | |||
There was a problem hiding this comment.
I would pass both index names here rather than taking dependency on the config.
| @@ -80,12 +81,18 @@ type ( | |||
| // GetVisibilityIndex return visibility index name from Elasticsearch config or empty string if it is not defined. | |||
| func (cfg *Config) GetVisibilityIndex() string { | |||
| if cfg == nil { | |||
| // Empty string is be used as default index name when Elasticsearch is not configured. | |||
There was a problem hiding this comment.
It was important comment. Why did you remove it? It had grammar errors though.
There was a problem hiding this comment.
I will add this back. But how is it supposed to work if user does not specify index name?
Comment on lines
43
to
55
| managerSelector interface { | ||
| readManager(namespace namespace.Name) manager.VisibilityManager | ||
| writeManagers() ([]manager.VisibilityManager, error) | ||
| } | ||
|
|
||
| sqlToESManagerSelector struct { | ||
| enableAdvancedVisibilityRead dynamicconfig.BoolPropertyFnWithNamespaceFilter | ||
| advancedVisibilityWritingMode dynamicconfig.StringPropertyFn | ||
| stdVisibilityManager manager.VisibilityManager | ||
| advVisibilityManager manager.VisibilityManager | ||
| } | ||
|
|
||
| esManagerSelector struct { |
There was a problem hiding this comment.
Please place all these structs and their methods in separate files. I hate to mix structs with methods in one file (I know we have many of those but it is just a legacy and I am constantly fixing it).
alexshtin
approved these changes
Jan 11, 2022
common/dynamicconfig/constants.go
Outdated
| @@ -383,8 +385,12 @@ const ( | |||
| AdvancedVisibilityPersistenceMaxWriteQPS | |||
| // AdvancedVisibilityWritingMode is key for how to write to advanced visibility | |||
| AdvancedVisibilityWritingMode | |||
| // EnableWriteToSecondaryAdvancedVisibility is the config to enable write to secondary visibility for elastic search | |||
There was a problem hiding this comment.
Suggested change
| // EnableWriteToSecondaryAdvancedVisibility is the config to enable write to secondary visibility for elastic search | |
| // EnableWriteToSecondaryAdvancedVisibility is the config to enable write to secondary visibility for Elasticsearch. |
common/dynamicconfig/constants.go
Outdated
| // EnableReadVisibilityFromES is key for enable read from elastic search | ||
| EnableReadVisibilityFromES | ||
| // EnableReadFromSecondaryAdvancedVisibility is the config to enable read from secondary elastic search |
There was a problem hiding this comment.
Suggested change
| // EnableReadFromSecondaryAdvancedVisibility is the config to enable read from secondary elastic search | |
| // EnableReadFromSecondaryAdvancedVisibility is the config to enable read from secondary Elasticsearch. |
yiminc
pushed a commit
to yiminc/temporal
that referenced
this pull request
Jan 12, 2022
* Change order by clauses in query and update V1 index template. Index created by the old template will continue to function, but won't see the query performance improvement. * Support visibility dual write to different ES indices. * Add additional dynamic configs to control visibility dual write behavior.
yiminc
pushed a commit
that referenced
this pull request
Jan 12, 2022
* Change order by clauses in query and update V1 index template. Index created by the old template will continue to function, but won't see the query performance improvement. * Support visibility dual write to different ES indices. * Add additional dynamic configs to control visibility dual write behavior.
johanforssell
added a commit
to johanforssell/temporal
that referenced
this pull request
Feb 8, 2023
PR temporalio#2359 added support for dual write to different ES indices. There was no change to the config template to enable this feature.
johanforssell
added a commit
to johanforssell/temporal
that referenced
this pull request
Feb 8, 2023
PR temporalio#2359 added support for dual write to different ES indices. There was no change to the config template to enable this feature. This commit will set secondary_visibility if env ES_SEC_VIS_INDEX is set.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Support visibility dual write to different ES indices
Add additional dynamic configs to control visibility dual write and read behavior
Why?
To support seamless switch to new ES visibility index
How did you test it?
Local test with 2 ES indices
Potential risks
No. The old index should only be deleted after the new index is verified working.
Is hotfix candidate?
No