Change update namespace to upsert custom search attributes #4080
Conversation
rodrigozhou
force-pushed
the
upsert-csa-update-ns
branch
from
9669784 to
cabac62
Compare
rodrigozhou
force-pushed
the
upsert-csa-update-ns
branch
2 times, most recently
from
4892c3e to
ed674da
Compare
rodrigozhou
force-pushed
the
upsert-csa-update-ns
branch
from
ed674da to
7926cf4
Compare
| } else if _, ok := current[key]; !ok { | ||
| result[key] = value | ||
| } else { | ||
| return nil, serviceerror.NewInvalidArgument( |
There was a problem hiding this comment.
hmm ask user to retry invalid argument looks weird.
Currently if there're concurrent namespace update, persistence layer will return Unavailable, which will be retried and customer most likely won't see the error.
There was a problem hiding this comment.
But this Unavailable error retry is in the grpc level, which won't work because the args will be the same from the operator handler.
The retry from the user will make operator handler to generate a new map to be upsert based on newer version of the namespace data.
There was a problem hiding this comment.
If user must change agrs, then InvalidArgument is the right error. Message should be different though. Don't use word "retry" there and explicitly say that specific argument must be changed.
| @@ -280,18 +277,22 @@ func (h *OperatorHandlerImpl) addSearchAttributesSQL( | |||
| if nsName == "" { | |||
| return errNamespaceNotSet | |||
| } | |||
| ns, err := h.namespaceRegistry.GetNamespace(namespace.Name(nsName)) | |||
| resp, err := client.DescribeNamespace( | |||
There was a problem hiding this comment.
Is this to avoid cache? Why not to call DescribeNamesapce from namespace handler directly?
There was a problem hiding this comment.
Yes, it's to avoid cache. Namespace handler is not available in this context.
There was a problem hiding this comment.
It can be added, but ok, let's leave it this way.
| } else if _, ok := current[key]; !ok { | ||
| result[key] = value | ||
| } else { | ||
| return nil, serviceerror.NewInvalidArgument( |
There was a problem hiding this comment.
If user must change agrs, then InvalidArgument is the right error. Message should be different though. Don't use word "retry" there and explicitly say that specific argument must be changed.
rodrigozhou
force-pushed
the
upsert-csa-update-ns
branch
from
7926cf4 to
db45583
Compare
rodrigozhou
force-pushed
the
upsert-csa-update-ns
branch
from
db45583 to
8e47773
Compare
|
Updated the operator/admin handler to return |
rodrigozhou
force-pushed
the
upsert-csa-update-ns
branch
from
8e47773 to
84c4045
Compare
| }, | ||
| }) | ||
| if err.Error() == errCustomSearchAttributeFieldAlreadyAllocated.Error() { |
There was a problem hiding this comment.
This is ugly. But I don't see a better way :-(
| @@ -280,18 +277,22 @@ func (h *OperatorHandlerImpl) addSearchAttributesSQL( | |||
| if nsName == "" { | |||
| return errNamespaceNotSet | |||
| } | |||
| ns, err := h.namespaceRegistry.GetNamespace(namespace.Name(nsName)) | |||
| resp, err := client.DescribeNamespace( | |||
There was a problem hiding this comment.
It can be added, but ok, let's leave it this way.
What changed?
DescribeNamespaceAPI: it fetches from persistence instead of cache.UpdateNamespaceto upsert the custom search attributes field to alias map instead of simply overwriting the entire map. If the value is an empty string (ie., the alias is an empty string), then remove the alias.Why?
UpdateNamespaceensures that concurrent calls to add search attributes are handled correctly without racing condition.How did you test it?
Started Server, and sent multiple calls to create search attribute using
tctland Temporal CLI. Verified that all search attributes were created correctly.Potential risks
No risks.
Is hotfix candidate?
No.