Namespace Management APIs #8
Conversation
| // the regions where the namespace is (or will be) located [optional] | ||
| repeated string passive_regions = 8; | ||
| // the export sink specifications keyed on the sink name [optional] | ||
| map<string, temporal.api.cloud.sink.v1.ExportSinkSpec> export_sinks = 9; |
There was a problem hiding this comment.
Do we want to make the export sinks a sub resource?
There was a problem hiding this comment.
For my particular use case, it would be nice to be able to reference a common export sink config since we use the same config globally across all namespaces (as history export is a platform-managed feature here). I don't feel particularly strongly here, however.
There was a problem hiding this comment.
@robzienert thanks for this feedback.
We are tracking this as a feature as part of the "hierarchy" story where various configurations like ca-certs, search-attrs, export configs can trickle down a hierarchy sub-tree as defaults...
cc: @jlacefie @sergeybykov
There was a problem hiding this comment.
TY - Rob. We released the first Export per Namespace with the understanding that some Namespace owners would like isolation at the S3 bucket. In a subsequent release we can provide Account level affordances for a default.
| // the web uri for the namespace | ||
| NamespaceURI uri = 6; | ||
| // the envelope is a list of service level agreements (SLAs) that can be provided around a given namespace | ||
| NamespaceEnvelope envelope = 7; |
There was a problem hiding this comment.
@robzienert TY for the comment here. What is exciting about this one for you? Also, do you have an opinion on the term Envelope in this context?
| message GetNamespacesRequest { | ||
| // The requested size of the page to retrieve | ||
| int32 page_size = 1; | ||
| // The page token |
There was a problem hiding this comment.
| // The page token | |
| // The page token if this is continuing from another response |
Not obvious to users that this is not required
| } | ||
|
|
||
| message GetNamespacesRequest { | ||
| // The requested size of the page to retrieve |
There was a problem hiding this comment.
May want to clarify the default (but don't have to)
There was a problem hiding this comment.
Added a default and a max to the comment.
| message NamespaceURI { | ||
| // The web ui address | ||
| string web = 1; | ||
| // The grpc address |
There was a problem hiding this comment.
| // The grpc address | |
| // The grpc host:port |
Just so people don't think it's a real URL with a scheme like web will be. Arguably this shouldn't even be named "grpc" because many users don't know/care that that happens to be our API protocol today, but meh.
| } | ||
|
|
||
| message ExportSink { | ||
| // The state of the sink |
There was a problem hiding this comment.
Mentioned before (so will stop mentioning), but please provide all possible string enumerate values if we're set on not using properly defined enums
| google.protobuf.Timestamp last_health_check_time = 5; | ||
| } | ||
|
|
||
| message S3Spec{ |
There was a problem hiding this comment.
| message S3Spec{ | |
| message S3Spec { |
|
|
||
| import "google/protobuf/timestamp.proto"; | ||
|
|
||
| message ExportSinkSpec { |
There was a problem hiding this comment.
Consider a unique, user-provided identifier for a sink (e.g. a name) if you want to support updating of these inside a list. Not required of course, but can be easier on you, the UI, and the user.
| // The regions where the namespace is (or will be) located - optional | ||
| repeated string passive_regions = 8; | ||
| // The export sink specifications keyed on the sink name - optional | ||
| map<string, temporal.api.cloud.sink.v1.ExportSinkSpec> export_sinks = 9; |
There was a problem hiding this comment.
Arguably "export" is too generic of a word for this. Is this just history export? Or is this also log export, audit export, user export, and all other exports we may support henceforth?
| // Environment of the namespace. - optional | ||
| // NOTE: currently there is no additional SLA or functional guarantee implied by the value of this field. | ||
| // supported environments: dev, test, prod | ||
| string environment = 6; |
There was a problem hiding this comment.
May we not include environment yet please? This seems premature and something that can be added later after we are aligned with the way we will expose hierarchy containers and metadata to users.
| } | ||
|
|
||
| message CodecServerPropertySpec { | ||
| // Server endpoints |
There was a problem hiding this comment.
nit - codec server endpoint
Also, does this take in 1 endpoint or many? the plural endpoints in the comment is ambiguous
There was a problem hiding this comment.
Its just one, fixed the comments.
| message CodecServerPropertySpec { | ||
| // Server endpoints | ||
| string endpoint = 1; | ||
| // Whether to pass access token, i.e. jwt |
There was a problem hiding this comment.
nit - comment should align to the UI to indicate where the JWT ends up.
UI comment - Pass the user access token with your endpoint.
There was a problem hiding this comment.
Ack fixed, as mentioned in the UI.
| string region = 1; | ||
| // The base64 encoded ca cert(s) that the clients can use for authentication and authorization | ||
| string accepted_client_ca = 2; | ||
| // The number of days the workflows data will be retained for |
There was a problem hiding this comment.
comment nit - align with the UI comment to indicate the impact of changing retention period
UI description
Changes to the retention period may impact your storage costs.
Any changes to the retention period will be applied to all new running workflows.
There was a problem hiding this comment.
ack fixed as suggested.
| repeated string passive_regions = 8; | ||
| } | ||
|
|
||
| message NamespaceURI { |
There was a problem hiding this comment.
Its the information that the customer will needs to locate the namespace's UI and the URL that the temporal client should connect to.
|
|
||
| import "google/protobuf/timestamp.proto"; | ||
|
|
||
| message ExportSinkSpec { |
There was a problem hiding this comment.
Where do users apply the sink message to a Namespace? It doesn't appear to be used in either the service or namespace protos? Please make sink an option when creating a namespace and provide users the ability to add a sink to a namespace at a later time.
| message CreateNamespaceRequest { | ||
| // The prefix to use for the namespace | ||
| // will create a namespace that's available at '<namespace_prefix>.<account>.tmprl.cloud:7233' | ||
| string namespace_prefix = 1; |
There was a problem hiding this comment.
why is the namespace_prefix not a part of the namespace spec?
how/where will users receive the "fully qualified" namespace name in this API spec as indicated in the comment of the prefix field?
There was a problem hiding this comment.
Yes makes sense to have the namespace_prefix in the spec. Moving to spec.
The fully qualified namespace name will be in the create-namespace response.
| string page_token = 2; | ||
| // Optional field to filter users by email address | ||
| // Filter users by email address - optional | ||
| string email = 3; |
There was a problem hiding this comment.
Completely unrelated, feel free to punt this comment for another day.
Should we have allowed a list of emails? Then this can operate as a batch Get operation (as opposed to just single email). Ditto for IDs.
There was a problem hiding this comment.
👍 At the very least this should clarify whether it's exact match or partial match
| int32 page_size = 1; | ||
| // The page token if this is continuing from another response. | ||
| // Optional, defaults to empty. | ||
| string page_token = 2; |
There was a problem hiding this comment.
Will we add more filters in a follow-up? Things like region (or regions) etc.
|
|
||
| message Namespace { | ||
| // The namespace identifier. | ||
| string namespace = 1; |
There was a problem hiding this comment.
I wonder if calling namespace in all these requests as id is clearer. Just because of the confusion around namespace name vs <name>.<account_id>.
That said, I understand that this aligns with what we call it in the OSS APIs.
There was a problem hiding this comment.
It's much less clear to try to use id because "namespace ID" is another thing. It is good that namespace aligns across all Temporal APIs.
| // The envelope is a list of service level agreements (SLAs) that can be provided around a given namespace. | ||
| OperatingEnvelope operating_envelope = 7; | ||
| // The private link for the namespace, if any. | ||
| oneof private_link { |
There was a problem hiding this comment.
In the future, its possible that we have a global namespace across clouds which uses both AWS private link and GCP private service connect (as an example). Or wireguard etc.
We may want to drop the oneof to ensure we aren't limited in the protocol.
There was a problem hiding this comment.
While it is a protobuf backwards compatible operation to move a field into a oneof later, it's often not a code-gen backwards compatible alteration. I do think it should be decided what compatibility guarantees you want to provide when you do provide them - wire guarantees or code-gen guarantees. If only the former, which is reasonable, then yes can wait on the oneof.
| string page_token = 2; | ||
| // Optional field to filter users by email address | ||
| // Filter users by email address - optional | ||
| string email = 3; |
There was a problem hiding this comment.
👍 At the very least this should clarify whether it's exact match or partial match
|
|
||
| import "google/protobuf/timestamp.proto"; | ||
|
|
||
| message CertificateFilterSpec { |
There was a problem hiding this comment.
Bump, unsure if this does exact string matches or partial string matches, and unsure whether any field must match or all fields must match.
| bool include_cross_origin_credentials = 3; | ||
| } | ||
|
|
||
| message GlobalSpec { |
There was a problem hiding this comment.
| message GlobalSpec { | |
| message GlobalNamespaceSpec { |
Usually I'd say you don't need the somewhat redundant Namespace but you need it here for two reasons: 1) just "global" for options has general meaning (one might think it's some cross-namespace global scope) and 2) the name of the product is "global namespace".
| repeated CertificateFilterSpec certificate_filters = 2; | ||
| } | ||
|
|
||
| message CodecServerSpec { |
There was a problem hiding this comment.
| message CodecServerSpec { | |
| message UICodecServerSpec { |
Mentioned before but now that I think about it, a user may assume this codec server will be used for everything instead of just the UI. Not a strong opinion here though.
There was a problem hiding this comment.
I don't know.. its totally possible that non ui client can use the endpoint field... the other fields don't make a lot of sense though.
| bool include_credentials = 3; | ||
| } | ||
|
|
||
| message NamespaceSpec { |
There was a problem hiding this comment.
Do I assume if it doesn't say optional then it's required? Arguably also saying something is required is simpler than me having to infer it by a lack of "optional".
| GlobalSpec global = 7; | ||
| } | ||
|
|
||
| message Endpoints { |
There was a problem hiding this comment.
Would totally support inlining this information in the Namespace if you wanted. Not a ton of value separating this out. But no strong opinion.
There was a problem hiding this comment.
I feel this will keep the Namespace object from getting too crowded. Also might be a good place to put other endpoints that might come up in the future. The rest endpoint that are coming along could go in here maybe...
| string resource_version = 2; | ||
| // The namespace specification. | ||
| NamespaceSpec spec = 3; | ||
| // The current state of the namespace. |
There was a problem hiding this comment.
If you are not going to use proper proto enumerates, please list all possible enumerates in doc for every one of these stringly-typed enumerates for the benefit of the caller.
| // The endpoints for the namespace. | ||
| Endpoints endpoints = 6; | ||
| // The envelope is a list of service level agreements (SLAs) that can be provided around a given namespace. | ||
| OperatingEnvelope operating_envelope = 7; |
There was a problem hiding this comment.
Just want to document here that I somewhat disagree with this seemingly Temporal-specific made up term compared to something like "limits", but no need to change anything
There was a problem hiding this comment.
+1 - I reached out to the crew-dynamic-namespace-envelop channel and asked if anyone has researched this. IMO, we create minor cognitive friction if we use uncommon terms for common concepts like quotas and limits
There was a problem hiding this comment.
asked @sergeybykov for input here as well. This feedback applies to the message definition on line 84 as well
recommend using quotas_and_limits
There was a problem hiding this comment.
Moved to calling Limits instead.
| // The envelope is a list of service level agreements (SLAs) that can be provided around a given namespace. | ||
| OperatingEnvelope operating_envelope = 7; | ||
| // The private link for the namespace, if any. | ||
| oneof private_link { |
There was a problem hiding this comment.
While it is a protobuf backwards compatible operation to move a field into a oneof later, it's often not a code-gen backwards compatible alteration. I do think it should be decided what compatibility guarantees you want to provide when you do provide them - wire guarantees or code-gen guarantees. If only the former, which is reasonable, then yes can wait on the oneof.
| } | ||
|
|
||
| // Get all regions | ||
| rpc GetRegions (GetRegionsRequest) returns (GetRegionsResponse) { |
There was a problem hiding this comment.
| rpc GetRegions (GetRegionsRequest) returns (GetRegionsResponse) { | |
| rpc GetRegions (GetRegionsRequest) returns (GetRegionsResponse) { |
Do you have a linter running? If not, I think it'd be time to add and it should be easy: https://linter.aip.dev/
|
Approving under the assumption that this is still technically a beta API that can have changes made as we learn more. |
What was changed
Why?
Checklist
Closes
How was this tested: