Support the new versioning API #1494

antlai-temporal · 2024-05-29T07:55:00Z

What was changed

This is a PR to support the new versioning API in the SDK-go that combines other (partially) reviewed PRs in the versioning-2
branch. In particular:

#1453
#1431
#1432
#1429
#1470

This PR will remain in draft mode until we can automate integration tests with CI, which requires a new docker image of the future server 1.24 (or a release candidate) for ubuntu, and a new CLI for mac/windows. Currently, all the integration tests pass using a local, up-to-date server (ubuntu).

However, I would appreciate early feedback to minimize delay on this important pre-release feature. Thanks for your help!

Checklist

Closes
Support the new versioning API #1420

…hanges

…ingRules (#1431)

…e in api (#1432)

* add new VersioningIntent options * BuildId -> BuildID * address comments

internal/client.go

internal/internal_versioning_client.go

internal/worker_version_sets.go

client/client.go

internal/internal_versioning_client.go

Quinn-With-Two-Ns · 2024-06-03T18:41:15Z

client/client.go

+	// (index 0). If the given index is too larger the rule will be
+	// inserted at the end of the list.
+	// WARNING: Worker versioning is currently experimental
+	VersioningOpInsertAssignmentRule = internal.VersioningOpInsertAssignmentRule


Generally initialisms in Go prefer constant case so either op or OP. I would prefer Operation here since op could be operation or option and without reading the docs I can't tell. @cretz any thoughts?

https://google.github.io/styleguide/go/decisions#initialisms

I was mostly following what we had with the old versioning, e.g., BuildIDOpAddNewIDInNewDefaultSet. I guess that strictly speaking Op is not an initialism, it is just an abbreviation, but we could make it OP. My only concern is that in the middle of a word sometimes it is harder to "parse", i.e., VersioningOPAddRedirectRule vs VersioningOpAddRedirectRule , is "OPA" or "PA" (you need look-ahead :-) ).

About the confusion with Options, we always have "Options" with no shortcuts, so that is less confusing with "OP" for operations. I worry if we extend operations it is just too much typing, e.g., VersioningOperationDeleteRedirectRule...

Anyway, happy to change things, @cretz comments?

I have no strong opinion. Operation works for me. I do appreciate how we're prefixing all of these w/ the same prefix though.

Quinn-With-Two-Ns · 2024-06-03T18:50:00Z

internal/client.go

+		//   - Backlog info for Workflow and/or Activity tasks
+		// When not supported by the server, it returns an empty [TaskQueueDescription]
+		// WARNING: Worker versioning is currently experimental, and requires server 1.24+
+		DescribeTaskQueueEnhanced(ctx context.Context, options *DescribeTaskQueueEnhancedOptions) (TaskQueueDescription, error)


I know the old new version took pointers for options, I think @cretz prefers it without pointers. The SDK is not consistent here, but we should try to be consistent for new APIs. I think without a pointer is probably nicer since we don't have to deal withnil.

Happy to change that, I saw 50/50 of APIs using pointers, and the old versioning always using it.
But if we do change it, we should also change the new UpdateWorkerVersioningRules and GetWorkerVersioningRules to not have pointers, so that all are consistent...
@cretz what do you think?

👍 Confirmed on Slack, without pointers is best IMO

cretz

LGTM, only a couple of minor comments

cretz · 2024-06-06T16:37:42Z

client/client.go

+	// PollerInfo provides information about a worker/client polling a task queue.
+	// It is used by [Client.TaskQueueTypeInfo].
+	// WARNING: Worker versioning is currently experimental.
+	PollerInfo = internal.PollerInfo


Can this be called TaskQueuePollerInfo or WorkerPollerInfo or something? I think PollerInfo is a bit too generic for this big package.

TaskQueuePollerInfo sounds great

cretz · 2024-06-06T16:38:18Z

client/client.go

+	// (index 0). If the given index is too larger the rule will be
+	// inserted at the end of the list.
+	// WARNING: Worker versioning is currently experimental
+	VersioningOpInsertAssignmentRule = internal.VersioningOpInsertAssignmentRule


I have no strong opinion. Operation works for me. I do appreciate how we're prefixing all of these w/ the same prefix though.

cretz · 2024-06-06T16:38:56Z

internal/client.go

+		//   - Backlog info for Workflow and/or Activity tasks
+		// When not supported by the server, it returns an empty [TaskQueueDescription]
+		// WARNING: Worker versioning is currently experimental, and requires server 1.24+
+		DescribeTaskQueueEnhanced(ctx context.Context, options *DescribeTaskQueueEnhancedOptions) (TaskQueueDescription, error)


👍 Confirmed on Slack, without pointers is best IMO

cretz · 2024-06-06T16:43:13Z

internal/internal_workflow_client.go

+//   - Backlog info for Workflow and/or Activity tasks
+//
+// WARNING: Worker versioning is currently experimental, and requires server 1.24+
+func (wc *WorkflowClient) DescribeTaskQueueEnhanced(ctx context.Context, options *DescribeTaskQueueEnhancedOptions) (TaskQueueDescription, error) {


It is a bit confusing to have half of versioning client code in this file and half in another, but I understand why. There is an argument to be made for a VersioningClient() similar to our ScheduleClient(), but I'm ok w/ this since it's only 3 methods. No need to change anything, just thinking out loud.

cretz · 2024-06-06T16:44:06Z

internal/worker_versioning_rules.go

I think all of this should go in internal_versioning_client.go instead of a new file, but not a super-strong opinion

ShahabT and others added 15 commits March 18, 2024 15:06

NOT FOR MERGE: Bump api-go version to include latest versioning API c…

3127dd7

…hanges

NOT FOR MERGE: Undo changes that we want to merge

7a9c619

Redo initial changes that we want to merge

d10ada0

NOT FOR MERGE: Update api-go version to latest

115af43

Support Update and Get for the new versioning rules (#1429)

c48e2a3

NOT FOR MERGE: Update api-go version to latest

50fca78

Change UpdateWorkerVersioningRules response to match GetWorkerVersion…

69259e2

…ingRules (#1431)

NOT FOR MERGE: Update api-go version to latest

b985605

Rename insert_compatible_redirect_rule to add_compatible_redirect_rul…

4a45118

…e in api (#1432)

NOT FOR MERGE: disable grpc dial lint errors

d9aa0b8

Update baseline to go sdk 1.26.1

34a08c8

Implement DescribeTaskQueueEnhanced (#1453)

e8147ac

Add new VersioningIntent options (#1470)

c7108f8

* add new VersioningIntent options * BuildId -> BuildID * address comments

Merge remote-tracking branch 'upstream/master' into versioning-2

a64c6e3

Clean up comments for PR

6481094

antlai-temporal requested a review from a team as a code owner May 29, 2024 07:55

antlai-temporal requested review from Quinn-With-Two-Ns, ShahabT and cretz May 29, 2024 07:58

antlai-temporal marked this pull request as draft May 29, 2024 07:59

Quinn-With-Two-Ns reviewed May 29, 2024

View reviewed changes

internal/client.go Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed May 29, 2024

View reviewed changes

internal/internal_versioning_client.go Show resolved Hide resolved

Quinn-With-Two-Ns reviewed May 29, 2024

View reviewed changes

internal/worker_version_sets.go Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed May 29, 2024

View reviewed changes

client/client.go Outdated Show resolved Hide resolved

Format deprecation

7807c94

Quinn-With-Two-Ns reviewed May 31, 2024

View reviewed changes

internal/internal_versioning_client.go Show resolved Hide resolved

Quinn-With-Two-Ns reviewed May 31, 2024

View reviewed changes

internal/internal_versioning_client.go Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed May 31, 2024

View reviewed changes

internal/internal_versioning_client.go Outdated Show resolved Hide resolved

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

View reviewed changes

Improve GoDocs

1f0d12f

antlai-temporal marked this pull request as ready for review June 6, 2024 16:39

cretz reviewed Jun 6, 2024

View reviewed changes

Rename constants, remove pointers in options

eadd267

cretz approved these changes Jun 6, 2024

View reviewed changes

antlai-temporal added 3 commits June 6, 2024 14:01

Merge remote-tracking branch 'upstream/master' into versioning-2

770a19e

NOT FOR MERGE: test CI with ubuntu docker image

aa37dff

Adding sleep for reachability cache in test

f76ddc4

Quinn-With-Two-Ns approved these changes Jun 6, 2024

View reviewed changes

antlai-temporal added 6 commits June 6, 2024 16:30

Replace Sleep by Eventually in test

f3c4daf

Remove extra test checks

ffc91bb

Undo 'Eventually' in test due to 1min cache TTL

7957957

Merge remote-tracking branch 'upstream/master' into versioning-2

ff4541c

Undo test CI with ubuntu docker image

ff82aaf

Skip new versioning tests with cli devserver

2b9a66f

antlai-temporal merged commit c73a007 into master Jun 7, 2024
14 checks passed

antlai-temporal deleted the versioning-2 branch June 7, 2024 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the new versioning API #1494

Support the new versioning API #1494

antlai-temporal commented May 29, 2024

Quinn-With-Two-Ns Jun 3, 2024

antlai-temporal Jun 3, 2024

cretz Jun 6, 2024

Quinn-With-Two-Ns Jun 3, 2024

antlai-temporal Jun 3, 2024

cretz Jun 6, 2024

cretz left a comment

cretz Jun 6, 2024

antlai-temporal Jun 6, 2024

cretz Jun 6, 2024

cretz Jun 6, 2024

cretz Jun 6, 2024

cretz Jun 6, 2024

Support the new versioning API #1494

Support the new versioning API #1494

Conversation

antlai-temporal commented May 29, 2024

What was changed

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment