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

Jump to bottom

Refactor configs for visibility #4095

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Refactor configs for visibility #4095

merged 1 commit into from
Apr 24, 2023

Conversation

rodrigozhou
Copy link
Contributor

@rodrigozhou rodrigozhou commented Mar 24, 2023 •
edited
Loading

What changed?

  • Introduce secondaryVisibilityStore config to set up secondary visibility (advancedVisibilityStore config will be deprecated)
  • Refactor dynamic configs related to visibility:
    • Consolidate configs to set max read/write QPS the existing ones that was fragmented by standard to advanced and es to es combinations: visibilityPersistenceMaxReadQPS and visibilityPersistenceMaxWriteQPS
    • Consolidate configs to select which visibility to read/write: enableReadFromSecondaryVisibility and secondaryVisibilityWritingMode.
  • Additional config validation to block non-supported dual visibility combinations.

Why?
Simplify all the different configs that was up to the dual visibility combination into simpler names.

How did you test it?
Tested on all supported dual visibility combinations involving standard visibility and SQL:

  • Cassandra -> SQL (advanced)
  • Cassandra -> ES
  • SQL (standard) -> SQL (advanced)
  • SQL (standard) -> ES (advanced)
  • SQL (advanced) -> SQL (advanced)

Potential risks

Is hotfix candidate?
No.

@rodrigozhou rodrigozhou requested a review from a team as a code owner March 24, 2023 00:35
@rodrigozhou rodrigozhou requested a review from alexshtin March 24, 2023 00:36
@rodrigozhou rodrigozhou force-pushed the new-dynamic-configs-visibility branch from a3e599c to b7c5e77 Compare March 24, 2023 16:08
@rodrigozhou rodrigozhou force-pushed the new-dynamic-configs-visibility branch from b7c5e77 to d4022d3 Compare March 28, 2023 17:38
alexshtin
alexshtin approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@alexshtin alexshtin left a comment

Choose a reason for hiding this comment

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

Please change our development configs and config_template.yaml to use new paramrters.

@rodrigozhou rodrigozhou force-pushed the new-dynamic-configs-visibility branch 2 times, most recently from b2bd975 to c6cdd9b Compare April 4, 2023 01:02
@rodrigozhou rodrigozhou force-pushed the new-dynamic-configs-visibility branch from c6cdd9b to 3a64af7 Compare April 19, 2023 19:14
@rodrigozhou rodrigozhou merged commit 12655c4 into temporalio:master Apr 24, 2023
@rodrigozhou rodrigozhou deleted the new-dynamic-configs-visibility branch April 24, 2023 16:40
@rodrigozhou rodrigozhou mentioned this pull request Apr 28, 2023
Merged
samanbarghi pushed a commit to samanbarghi/temporal that referenced this pull request May 2, 2023
@rodrigozhou @samanbarghi
Refactor configs for visibility (temporalio#4095)
c5dbc87 
New dynamic configs for visibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexshtin alexshtin alexshtin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rodrigozhou @alexshtin