-
Notifications
You must be signed in to change notification settings - Fork 430
feat: add support for secondary visibility store (dual visibility) #839
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.
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
feat: add support for secondary visibility store (dual visibility) #839
Conversation
|
|
Add support for secondaryVisibilityStore configuration to enable dual visibility feature in Temporal server. This allows writing to two visibility stores simultaneously for migration scenarios. Changes: - Add secondaryVisibilityStore support in temporal.persistence.filterConfig - Include secondaryVisibility in temporal.persistence.eachStore iteration - Add TEMPORAL_SECONDARY_VISIBILITY_STORE_PASSWORD env var to server pods - Automatically process secondary visibility store in schema jobs This implementation adapts PR temporalio#803 to the V1 chart architecture, leveraging the centralized helper functions introduced in V1 to avoid code duplication. Resolves: temporalio#803
Add helm-unittest tests to verify: - ConfigMap contains secondaryVisibilityStore configuration - Secondary visibility password uses correct env var placeholder - Environment variable is injected when secondaryVisibilityStore is configured - Environment variable is not injected when secondaryVisibilityStore is absent
Remove redundant check for in the conditional. When is empty, eq comparison will be false, making the extra 'and' check unnecessary.
Make getStoreByType return an empty dict when the store isn't configured, removing the need for special-case logic in eachStore. This treats secondaryVisibility consistently with other stores. This change: - Adds a check in getStoreByType to return empty dict if storeName is nil - Simplifies eachStore to use getStoreByType for all stores including secondary - Makes secondary visibility less of an exceptional case
1c6bb3f to
b222fc9
Compare
Remove unresolved conflict markers
…ment Use getStoreByType for secondaryVisibility to be consistent with default and visibility stores. This removes special-case logic and makes the code cleaner by: - Using getStoreByType which returns empty dict when not configured - Checking $secondaryVisibilityStore variable instead of .Values path
|
Looks good, please resolve conflicts and then I can get this merged. |
|
Hey @ignaciogonzalezsomoza, please could you merge main, then I can get your PR merged :) |
Keep both tests: - Secondary visibility store for dual visibility (from this PR) - Metrics config (from upstream)
The secret name should include 'temporal-' prefix to match the template output.
|
|
|
🎉 |
Summary
This PR adds support for
secondaryVisibilityStoreconfiguration to enable the dual visibility feature in Temporal server. This allows writing to two visibility stores simultaneously, which is useful for migration scenarios (e.g., migrating from Cassandra to Elasticsearch for visibility).Changes
secondaryVisibilityStoresupport intemporal.persistence.filterConfigfunctiontemporal.persistence.eachStoreto include secondary visibility store in iterationTEMPORAL_SECONDARY_VISIBILITY_STORE_PASSWORDenvironment variable to server podssecondaryVisibilityStoreis configuredImplementation Details
This implementation leverages the V1 chart architecture's centralized helper functions, avoiding the code duplication present in PR #803 (which targeted v0.x). The key improvements:
temporal.persistence.filterConfigtemporal.persistence.eachStoresecondaryVisibilityStoreconfigured continue to work as beforeTesting
Tested in a development cluster with:
temporal_visibility_v1_dev)temporal_visibility_v1_dev_readonly)Verified that:
Related
Closes #803
This PR adapts the changes from #803 to the V1 chart structure, making them compatible with the refactored architecture introduced in the V1 release.
Documentation
Example configuration: