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.

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 GetIndexName method to visibility manager #3820

Merged

Conversation

rodrigozhou
Copy link
Contributor

What changed?
Adding a interface to get the visibility storage schema name, ie., for ES the index name, and for SQL DB the table name.

Why?
It will allow to abstract the schema name without needing to read from config. The visibility manager already is an abstraction of the underlying storage, so it makes it easier to get the schema name.

How did you test it?
Unit tests

Potential risks
Maybe will have some issue with cluster metadata when using SQL DB since it was using empty string as "index name".

Is hotfix candidate?
No.

@rodrigozhou rodrigozhou requested a review from alexshtin January 20, 2023 03:04
@rodrigozhou rodrigozhou requested a review from a team as a code owner January 20, 2023 03:04
@rodrigozhou rodrigozhou force-pushed the vis-manager-get-index-name branch 2 times, most recently from 306e25d to 1897988 Compare January 23, 2023 18:53
@rodrigozhou rodrigozhou force-pushed the vis-manager-get-index-name branch from 1897988 to fe9114b Compare January 23, 2023 21:07
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.

I would call it StoreName rather than IndexName but we can consider entire visibility as "index" therefore IndexName makes sense even for non-ES persistences.

@rodrigozhou rodrigozhou force-pushed the vis-manager-get-index-name branch 4 times, most recently from cea02f1 to a50f1b0 Compare January 26, 2023 23:15
@rodrigozhou rodrigozhou force-pushed the vis-manager-get-index-name branch from a50f1b0 to 3b7973c Compare January 27, 2023 08:15
@rodrigozhou rodrigozhou merged commit dc257ce into temporalio:master Jan 27, 2023
@rodrigozhou rodrigozhou deleted the vis-manager-get-index-name branch January 27, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants