Add custom search attribute aliases map to namespace config #3866
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
approved these changes
Jan 30, 2023
proto/internal/temporal/server/api/persistence/v1/namespaces.proto
Outdated
Show resolved
Hide resolved
| @@ -89,6 +100,10 @@ func FromPersistentState(record *persistence.GetNamespaceResponse) *Namespace { | |||
| isGlobalNamespace: record.IsGlobalNamespace, | |||
| failoverNotificationVersion: record.Namespace.FailoverNotificationVersion, | |||
| notificationVersion: record.NotificationVersion, | |||
| customSearchAttributesMapper: CustomSearchAttributesMapper{ | |||
There was a problem hiding this comment.
It is every 10s for every namespace. Just FYI.
There was a problem hiding this comment.
Hm. Maybe follow up optimization to avoid calling this if there are no changes.
MichaelSnowden
suggested changes
Jan 30, 2023
common/namespace/namespace.go
Outdated
Comment on lines
313
to
331
| func (m *CustomSearchAttributesMapper) GetAlias(fieldName string, namespace string) (string, error) { | ||
| alias, ok := m.fieldToAlias[fieldName] | ||
| if !ok { | ||
| return "", serviceerror.NewInvalidArgument( | ||
| fmt.Sprintf("Namespace %s has no mapping defined for field name %s", namespace, fieldName), | ||
| ) | ||
| } | ||
| return alias, nil | ||
| } | ||
|
|
||
| func (m *CustomSearchAttributesMapper) GetFieldName(alias string, namespace string) (string, error) { | ||
| fieldName, ok := m.aliasToField[alias] | ||
| if !ok { | ||
| return "", serviceerror.NewInvalidArgument( | ||
| fmt.Sprintf("Namespace %s has no mapping defined for search attribute %s", namespace, fieldName), | ||
| ) | ||
| } | ||
| return fieldName, nil | ||
| } |
There was a problem hiding this comment.
- We should change the return type to (string, bool) because these are utility functions and the service-level functions exist higher up in the call stack
- We should remove the namespace parameter because it is not used for functionality here. Callers can dictate what the error should be and if a namespace is attached to it.
There was a problem hiding this comment.
For both asks, we need this way because it's part of the searchattribute.Mapper interface.
Maybe in another follow up PR to improve it.
| @@ -290,3 +309,27 @@ func (n Name) String() string { | |||
| func (n Name) IsEmpty() bool { | |||
| return n == EmptyName | |||
| } | |||
|
|
|||
| func (m *CustomSearchAttributesMapper) GetAlias(fieldName string, namespace string) (string, error) { | |||
There was a problem hiding this comment.
Consider using a type alias here so that callers don't suffer from type blindness since "fieldName" and "alias" are both strings but have very different meanings
rodrigozhou
force-pushed
the
add-search-attributes-alias-namespace-config
branch
from
364aaf4 to
cbdb8d7
Compare
rodrigozhou
changed the title
rodrigozhou
force-pushed
the
add-search-attributes-alias-namespace-config
branch
from
cbdb8d7 to
91e53d5
Compare
MichaelSnowden
approved these changes
Jan 30, 2023
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?
Added custom search attributes alias mapper to namespace object.
Why?
Custom search attributes alias map is per namespace and stored in the namespace config.
This adds a mapper struct that implements
searchattribute.Mapperinterface to namespace object for easy usage.How did you test it?
Unit tests.
Potential risks
None, currently not in use.
Is hotfix candidate?
No.