Add namespace level capabilities #341

Quinn-With-Two-Ns · 2024-01-09T17:42:22Z

Add a message to NamespaceInfo to include namespace level capabilities. Different then the capabilities returned by GetSystemInfoResponse because those are scoped to a cluster where these are scoped to a namespace. I included the field in NamespaceInfo since that is exposed for all users already and is already been used for similar feature flags in a less general way. This is scoped to server configs, not to include cloud account level configs.

Is this intended to work in OS and Cloud

Yes

Who is intended to use this

Applications like UI (Not the SDKs internally)

temporal/api/namespace/v1/message.proto

cretz · 2024-01-09T20:16:32Z

temporal/api/namespace/v1/message.proto

@@ -45,6 +45,14 @@ message NamespaceInfo {
    // A key-value map for any customized purpose.
    map<string, string> data = 5;
    string id = 6;
+    // All capabilities the namespace supports.
+    Capabilities capabilities = 7;


Can you confirm whether cloud supports DescribeNamespace and therefore can also add capabilities here? If not, is this intentionally OSS only?

Cloud supports DescribeNamespace . This feature should work in OSS and Cloud.

Can you confirm with cloud team before merging that this is acceptable to them?

cloud does not censor the namespaceInfo object. Don't see a reason to censor capabilities at this point in time, so LGTM

cretz · 2024-01-09T20:18:06Z

temporal/api/namespace/v1/message.proto

@@ -45,6 +45,14 @@ message NamespaceInfo {
    // A key-value map for any customized purpose.
    map<string, string> data = 5;
    string id = 6;
+    // All capabilities the namespace supports.
+    Capabilities capabilities = 7;


Do all SDKs make a describe namespace call so they can get this kind of information? What if there are client capabilities? Even in the Go SDK doesn't call this until workers are created correct? I wonder if there are better ways to relay namespace-specific capabilities (maybe accepting a namespace on GetSystemInfo?)

Currently GetSystemInfo is not a namespace call. I think we'd need server and cloud to chime in on the scope of making it conditionally authorized by namespace.

My fear is that not all SDKs make this call, and the ones that do only do it before worker start not at client dial time (because we allow people to use another namespace on the same client connection without any network calls in between). I think we can call this WorkerCapabilities and feel a bit safer if whatever idea you have for putting in this field are worker specific.

So I was not expecting the SDKs to be calling this at client dial time or using it to make any SDK internal decisions. @bergundy you opened the issue here What was your expectation of the use of this API?

I don't expect the SDK to invoke this call.
I expect applications to use it though.

One of the use cases I had in mind is our features repo or other test frameworks that are designed to work with arbitrary server to disable certain tests when features are not implemented by the targeted server.

The UI is another example, it can use these capabilities to as feature flags.

OK that was my expectation as well

👍 If we add those initial three capabilities listed later, that works for me.

bergundy · 2024-01-10T01:13:59Z

temporal/api/namespace/v1/message.proto

+
+    // Namespace capability details. Should contain what features are enabled in a namespace.
+    message Capabilities {
+    }


You can already add eager_workflow_start, sync_update and async_update as the first set of capabilities.

To confirm, it is ok if the SDK never calls this?

Yeah, the SDK will get errors if it tries to use these APIs, the capabilities are meant to be used at the application layer.

cretz

LGTM, but can you confirm with cloud that this is ok with them?

dnr · 2024-01-23T18:39:45Z

temporal/api/namespace/v1/message.proto

    // Whether scheduled workflows are supported on this namespace. This is only needed
    // temporarily while the feature is experimental, so we can give it a high tag.


This one was an attempt at starting a list of capabilities. Maybe it should be moved into the new structure?

Also do we want to include versioning apis also? (we can have two fields for "v1" and "v2" since we're changing them)

I don't think it is worth moving it at this point

I can include versioning APIs, but the items I have in this PR are just initial features we can always add more

Okay. Looks good for now then

dnr · 2024-01-23T22:37:04Z

temporal/api/namespace/v1/message.proto

    // Whether scheduled workflows are supported on this namespace. This is only needed
    // temporarily while the feature is experimental, so we can give it a high tag.


Okay. Looks good for now then

Quinn-With-Two-Ns requested review from a team as code owners January 9, 2024 17:42

cretz reviewed Jan 9, 2024

View reviewed changes

bergundy reviewed Jan 10, 2024

View reviewed changes

cretz approved these changes Jan 16, 2024

View reviewed changes

Quinn-With-Two-Ns force-pushed the ns-features branch from b5733db to 31e523e Compare January 23, 2024 18:18

dnr reviewed Jan 23, 2024

View reviewed changes

dnr approved these changes Jan 23, 2024

View reviewed changes

Quinn-With-Two-Ns added 3 commits January 30, 2024 08:28

Add namespace level capabilities

9cc377c

remove valid_capabilities

8e80af9

Add first few namespace capabilities

368b973

Quinn-With-Two-Ns force-pushed the ns-features branch from 31e523e to 368b973 Compare January 30, 2024 16:28

Quinn-With-Two-Ns merged commit 34e5d97 into temporalio:master Jan 30, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace level capabilities #341

Add namespace level capabilities #341

Quinn-With-Two-Ns commented Jan 9, 2024 •

edited

Loading

cretz Jan 9, 2024

Quinn-With-Two-Ns Jan 9, 2024

cretz Jan 16, 2024

mastermanu Jan 29, 2024

cretz Jan 9, 2024

Quinn-With-Two-Ns Jan 9, 2024

cretz Jan 9, 2024

Quinn-With-Two-Ns Jan 10, 2024

bergundy Jan 10, 2024

Quinn-With-Two-Ns Jan 10, 2024

cretz Jan 10, 2024

bergundy Jan 10, 2024

cretz Jan 10, 2024

bergundy Jan 10, 2024

cretz left a comment

dnr Jan 23, 2024

Quinn-With-Two-Ns Jan 23, 2024

dnr Jan 23, 2024

dnr Jan 23, 2024

		// Whether scheduled workflows are supported on this namespace. This is only needed
		// temporarily while the feature is experimental, so we can give it a high tag.

Add namespace level capabilities #341

Add namespace level capabilities #341

Conversation

Quinn-With-Two-Ns commented Jan 9, 2024 • edited Loading

Is this intended to work in OS and Cloud

Who is intended to use this

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Jan 9, 2024 •

edited

Loading