Redo search attribute management apis #37
Conversation
anekkanti
commented
Aug 7, 2024
anekkanti
commented
- Deprecates the search attribute in the namespace resource.
- Introduces new set of search attribute APIs.
| STATE_ADDING= 1; // The region is being added to the namespace. | ||
| STATE_ACTIVE= 2; // The namespace is active in this region. |
There was a problem hiding this comment.
| STATE_ADDING= 1; // The region is being added to the namespace. | |
| STATE_ACTIVE= 2; // The namespace is active in this region. | |
| STATE_ADDING = 1; // The region is being added to the namespace. | |
| STATE_ACTIVE = 2; // The namespace is active in this region. |
| STATE_FAILED = 3; // The operation failed, check failure_reason for more details. | ||
| STATE_CANCELLED = 4; // The operation was cancelled. | ||
| STATE_FULFILLED = 5; // The operation was fulfilled. | ||
| STATE_ADDING= 1; // The region is being added to the namespace. |
There was a problem hiding this comment.
The changes to this enum seem unrelated to the current PR, is it intentional?
| // The type of the search attribute cannot be changed once set. | ||
| SearchAttributeType type = 2; | ||
|
|
||
| enum SearchAttributeType { |
There was a problem hiding this comment.
You are inconsistent with how you prefix your inner enums/types. You have SearchAttributeSpec.SearchAttributeType here and SearchAttribute.State below.
| } | ||
|
|
||
| // Add a new search attribute to a namespace. | ||
| rpc AddSearchAttribute(AddSearchAttributeRequest) returns (AddSearchAttributeResponse) { |
There was a problem hiding this comment.
| rpc AddSearchAttribute(AddSearchAttributeRequest) returns (AddSearchAttributeResponse) { | |
| rpc AddSearchAttribute (AddSearchAttributeRequest) returns (AddSearchAttributeResponse) { |
This spacing is inconsistent (throughout this file, I just haven't really mentioned it before)
| string namespace = 1; | ||
| // The requested size of the page to retrieve - optional. | ||
| // Cannot exceed 1000. Defaults to 100. | ||
| int32 page_size = 2; |
There was a problem hiding this comment.
I understand the desire to match other pagination here, but I can't see a world where so many search attributes would come back where pagination would be needed. And then if we accept that you'll always get the full set of search attributes on every call, it is much easier on users if search attributes are a map.
Having said that, I am ok with this too, just wanted to bring it up.
|
|
||
| enum State { | ||
| STATE_UNSPECIFIED = 0; | ||
| STATE_ADDING = 1; // The search attribute is being added. |
There was a problem hiding this comment.
Confirmed in descriptor that trailing comments on the same line are treated as comments for the item same as if they are above, but I think it's more consistent to always put comments for a construct above the construct.
