Add deployment API #1733

antlai-temporal · 2024-12-02T02:55:11Z

What was changed

This PR implements the new deployment API needed for worker versioning. It provides a new DeploymentClient that has methods:

Describe(ctx context.Context, deploymentID Deployment) (DeploymentInfo, error)
List(ctx context.Context, options DeploymentListOptions) (DeploymentListIterator, error)
GetReachability(ctx context.Context, deploymentID Deployment) (DeploymentReachabilityInfo, error)
GetCurrent(ctx context.Context, seriesName string) (DeploymentInfo, error)
SetCurrent(ctx context.Context, options DeploymentSetCurrentOptions) (DeploymentSetCurrentResponse, error)

Checklist

Closes
Support the Deployment API #1713
How was this tested:

Unit tests following the ScheduleClient template. System tests in a separate PR when there is a server that supports it.

cretz

Mostly LGTM, one slight suggestion

cretz · 2024-12-02T13:27:01Z

internal/deployment_client.go

+	DeploymentClient interface {
+		// Describes an existing deployment.
+		// NOTE: Experimental
+		Describe(ctx context.Context, deploymentID Deployment) (DeploymentInfo, error)


I wonder if for Describe and GetCurrent you'd rather have options structures just to be future proof (Go is a bit annoying here w/ no overloads or optional params).

Makes sense, it will be a bit more annoying to use but it is worth avoiding a DescribeWithOptions...

~~Deployment is already a struct, so that will give us a escape hatch if we need a deployment id with other fields.~~ Never mind. we may want other options that limit what Describe or GetReachability shows, adding wrappers too...
Adding DeploymentGetCurrentOptions that was just a string argument...

Quinn-With-Two-Ns · 2024-12-02T15:13:10Z

internal/deployment_client.go

+		Deployment Deployment
+
+		// ClientIdentity - Optional: The identity of the client who initiated this request.
+		ClientIdentity string


Why are we not using the client identity , I think that is what every other call uses.

Good point, I'll send the client identitiy as default, they may want to override it for versioning...

No other call in the SDK needed an override, I would leave off until there is an ask

Quinn-With-Two-Ns · 2024-12-02T15:19:38Z

internal/deployment_client.go

+		// UpsertEntries - Metadata entries inserted or modified. When values are not
+		// of type *commonpb.Payload, the default data converter will be used to generate
+		// payloads.
+		UpsertEntries map[string]interface{}


the default data converter will be used to generate payloads.

Why? does the Temporal server need to read these?

The idea of payloads was to support encryption. Converting to payloads transparently in UpsertEntries was just for convenience, they can always pass payloads if they want...

We should use the clients data converter here then

We should only use the default data converter if the server needs to read the payload

I'm using converter.GetDefaultDataConverter().ToPayload, I thought that would give me client data converter, which one should I be using?

I see getDataConverterFromWorkflowContext() may give me what I need, it defaults to the default data converter if none provided...

WorkflowClient.dataConverter

sdk-go/internal/internal_workflow_client.go

Line 94 in 158b823

dataConverter converter.DataConverter

GetDefaultDataConverter() is a program wide data converter, not the clients

Server does not need to read these.

Quinn-With-Two-Ns · 2024-12-03T01:13:43Z

internal/client.go

@@ -417,6 +417,9 @@ type (
 		// Schedule creates a new shedule client with the same gRPC connection as this client.
 		ScheduleClient() ScheduleClient

+		// DeploymentClient creates a new deployment client with the same gRPC connection as this client.
+		DeploymentClient() DeploymentClient


nit: should be experimental

ShahabT · 2024-12-04T22:09:52Z

internal/deployment_client.go

+		CreateTime time.Time
+
+		// Current - Whether this deployment is the current one for its deployment series.
+		Current bool


Nit: wouldn't IsCurrent be more more idiomatic?

ShahabT · 2024-12-04T22:10:16Z

internal/deployment_client.go

+		CreateTime time.Time
+
+		// Whether this deployment is the current one for its deployment series.
+		Current bool


ShahabT · 2024-12-04T22:15:15Z

internal/deployment_client.go

+
+		// LastUpdateTime - When reachability was last computed. Computing reachability
+		// is an expensive operation, and the server caches results.
+		LastUpdateTime time.Time


hmm.. if we embed DeploymentInfo there would be timestamps such CreateTime besides this LastUpdateTime, user wouldn't know if the field only refers to reachability status update time. Later more timestamps and other fields will be added to Deployment Info (and maybe to Reachability response) and I worry it'd be confusing for users if we mix them. Should we make deployment info just a nested field instead of an embed?

Add deployment API

e470c0f

antlai-temporal requested a review from a team as a code owner December 2, 2024 02:55

cretz reviewed Dec 2, 2024

View reviewed changes

Quinn-With-Two-Ns reviewed Dec 2, 2024

View reviewed changes

Add options struct to methods and use client data converter

7129c7b

Quinn-With-Two-Ns reviewed Dec 3, 2024

View reviewed changes

Quinn-With-Two-Ns approved these changes Dec 3, 2024

View reviewed changes

antlai-temporal added 2 commits December 2, 2024 17:46

Removing ClientIdentity from options

a650a65

Fix unit test

630a5b0

antlai-temporal merged commit 34b1096 into temporalio:versioning-3 Dec 3, 2024
12 checks passed

ShahabT reviewed Dec 4, 2024

View reviewed changes

antlai-temporal mentioned this pull request Dec 5, 2024

Versioning 3 #1744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deployment API #1733

Add deployment API #1733

antlai-temporal commented Dec 2, 2024

cretz left a comment

cretz Dec 2, 2024

antlai-temporal Dec 2, 2024

antlai-temporal Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 2, 2024

antlai-temporal Dec 2, 2024

Quinn-With-Two-Ns Dec 3, 2024

Quinn-With-Two-Ns Dec 2, 2024

antlai-temporal Dec 2, 2024

Quinn-With-Two-Ns Dec 2, 2024

Quinn-With-Two-Ns Dec 2, 2024 •

edited

Loading

antlai-temporal Dec 2, 2024

antlai-temporal Dec 2, 2024

Quinn-With-Two-Ns Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 2, 2024

ShahabT Dec 2, 2024

antlai-temporal Dec 2, 2024

Quinn-With-Two-Ns Dec 3, 2024

ShahabT Dec 4, 2024

ShahabT Dec 4, 2024

ShahabT Dec 4, 2024

Add deployment API #1733

Add deployment API #1733

Conversation

antlai-temporal commented Dec 2, 2024

What was changed

Checklist

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

antlai-temporal Dec 2, 2024 • edited Loading

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

Quinn-With-Two-Ns Dec 2, 2024 • edited Loading

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 Dec 2, 2024 • edited Loading

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

antlai-temporal Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 2, 2024 •

edited

Loading