Proto and persistence definitions for replicated versioning #4172

bergundy · 2023-04-14T21:54:47Z

While designing replication for the worker versioning feature, we realized we need to make some schema changes and add a new table to track user provided data for a task queue.
When a new cluster comes online, we'll need to seed it with data from the task_queue_user_data table.
This table will be scanned and replication tasks will be generated from each entry in the table.

bergundy · 2023-04-14T21:56:26Z

go.mod

@@ -142,3 +142,8 @@ require (
 	modernc.org/strutil v1.1.3 // indirect
 	modernc.org/token v1.1.0 // indirect
 )
+
+replace (


NOTE that I'm merging this PR into the worker-versioning feature branch, we should keep these replacements until this Go SDK change is published and we can merge the API changes.

bergundy · 2023-04-14T21:57:31Z

I know I'll need to provide a schema upgrade script for adding these new tables but for now let's just make sure that the definitions are correct.

bergundy · 2023-04-14T22:10:24Z

proto/internal/temporal/server/api/matchingservice/v1/request_response.proto

-        // If the request's hash matched, this variant is set (and will be true).
-        bool matched_req_hash = 2;
+        temporal.server.api.persistence.v1.TaskQueueUserData user_data = 1;
+        int64 user_data_version = 2;


I need to change this to a bool

I'll remove this file from the PR, I'll add in a follow up PR though.

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

Sushisource · 2023-04-14T23:45:57Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+
+// Container for all persistent user provided data for a task queue.
+message TaskQueueUserData {
+    // Updated every time a new timestamp is generated to ensure the semantics of the Hybrid Logical Clock.


A bit confusing. "every time a new timestamp is generated" for what purpose?

Something like "Updated whenever any contained data is modified" is more straightforward, unless that's inaccurate in some way I don't follow.

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

Sushisource · 2023-04-14T23:48:41Z

schema/cassandra/temporal/schema.cql

+  task_queue_name text,
+  data            binary, -- temporal.server.api.persistence.v1.TaskQueueUserData as proto binary blob
+  version         int64,  -- Version of this row, used for optimistic concurrency
+  updated_at      timestamp,


Why do we need the timestamp here if it's in the blob? For sorting or something?

Including the version column, it's duplicative of the HLC in the blob. Might we only have it here, and not need it in the blob?

It can't be used for sorting really..
I initially wanted to use this when seeding the a new cluster with replication tasks to verify the table isn't updated in the meantime but I think it could also be useful as general metadata on the table.

Might we remove it from the blob itself then?

It's not the same time, here's it's wall clock and on the struct it's a cluster local HLC.
Also note that the HLC only progresses when values are generated due to user interaction, not when applying replication events.

I'm also confused at why there are both and what the function of each one is

I'll just leave this out and see if we need it later.
I was thinking we could use this timestamp to verify that no new items are added after we seed the replication queue when a new cluster comes online.

…rsioning branch

Co-authored-by: Spencer Judge <sjudge@hey.com>

dnr

there's lots of unrelated stuff in the diff.. can you rebase the branch manually (push without a PR) and then rebase this PR on top of that to have only the versioning-related stuff?

dnr · 2023-04-18T05:34:44Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+    temporal.server.api.clock.v1.HybridLogicalClock state_update_timestamp = 3;
+}
+
+// An internal represenation of https://github.com/temporalio/api/blob/9206f1556469515d8e5a82c4d4bf21f2dd9730ce/temporal/api/taskqueue/v1/message.proto#L92


write the fully qualified name instead of linking to a random commit?

Yeah, will do.

dnr · 2023-04-18T05:36:33Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+// An internal represenation of https://github.com/temporalio/api/blob/9206f1556469515d8e5a82c4d4bf21f2dd9730ce/temporal/api/taskqueue/v1/message.proto#L92
+message CompatibleVersionSet {
+    // Set IDs are used internally by matching.
+    // The first set ID for a given set is the first added build ID.


no it's not, or at least not directly. just leave this out? it's not required to describe the semantics

Sure, I'll just leave it out.
I think it should be the first build ID, but let's discuss off PR.

it can be a hash of the first build id, perhaps. but no need to describe that here

Removed this line

dnr · 2023-04-18T05:37:32Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+    // HLC timestamp representing when the state was updated or the when build ID was originally inserted.
+    // (-- api-linter: core::0142::time-field-type=disabled
+    //     aip.dev/not-precedent: Using HLC instead of wall clock. --)
+    temporal.server.api.clock.v1.HybridLogicalClock state_update_timestamp = 3;


we don't use these timestamps for anything other than delete, right? should we consider having this be a dual-purpose field: "if absent, id is active. if present, it's deleted with this timestamp." ?

No, because if a cluster rejects the deletion it may "resurrect" the build ID by setting a newer timestamp and state to ACTIVE.

ah, okay. I guess you can do it by having two fields for an update timestamp and a delete timestamp, but there's no real advantage of that over this

dnr · 2023-04-18T05:39:45Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+    // Updated whenever user data is directly updated due to a user action but not when applying replication events.
+    // The clock is referenced when new timestamps are generated to ensure it produces monotonically increasing
+    // timestamps.
+    temporal.server.api.clock.v1.HybridLogicalClock clock = 1;


what exactly is this one used for again? I thought the timestamps within versioning_data are enough to merge

This keeps the last clock in order to produce a new clock on update.
We need this to ensure that the clock is monotonically increasing in a single cluster context.

oh right. when I read reading above I was wondering where it stores the last used timestamp to avoid duplicates, but I didn't connect this to that. to be nitpicky: the comment above should say it's the last recorded cluster-local HLC timestamp for this task queue. another task queue might have a later one recorded (due to clock skew) but we never compare them across task queues so it doesn't matter.

dnr · 2023-04-18T05:40:55Z

schema/cassandra/temporal/schema.cql

+CREATE TABLE task_queue_user_data (
+  namespace_id    uuid,
+  task_queue_name text,
+  data            binary, -- temporal.server.api.persistence.v1.TaskQueueUserData as proto binary blob


add data_encoding and change the above comment to not say it's a proto

Did this for the sql tables and forgot cassandra, thanks!

schema/cassandra/temporal/schema.cql

dnr · 2023-04-18T05:43:53Z

schema/cassandra/temporal/schema.cql

+  task_queue_name text,
+  data            binary, -- temporal.server.api.persistence.v1.TaskQueueUserData as proto binary blob
+  version         int64,  -- Version of this row, used for optimistic concurrency
+  updated_at      timestamp,


I'm also confused at why there are both and what the function of each one is

bergundy · 2023-04-18T16:42:10Z

there's lots of unrelated stuff in the diff.. can you rebase the branch manually (push without a PR) and then rebase this PR on top of that to have only the versioning-related stuff?

Yeah, I updated the PR branch and forgot to update the base branch... first time trying this method...

dnr · 2023-04-19T05:56:50Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+    // HLC timestamp representing when the state was updated or the when build ID was originally inserted.
+    // (-- api-linter: core::0142::time-field-type=disabled
+    //     aip.dev/not-precedent: Using HLC instead of wall clock. --)
+    temporal.server.api.clock.v1.HybridLogicalClock state_update_timestamp = 3;


ah, okay. I guess you can do it by having two fields for an update timestamp and a delete timestamp, but there's no real advantage of that over this

dnr · 2023-04-19T06:17:37Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+// An internal represenation of https://github.com/temporalio/api/blob/9206f1556469515d8e5a82c4d4bf21f2dd9730ce/temporal/api/taskqueue/v1/message.proto#L92
+message CompatibleVersionSet {
+    // Set IDs are used internally by matching.
+    // The first set ID for a given set is the first added build ID.


it can be a hash of the first build id, perhaps. but no need to describe that here

dnr · 2023-04-19T06:21:39Z

proto/internal/temporal/server/api/persistence/v1/task_queues.proto

+    // Updated whenever user data is directly updated due to a user action but not when applying replication events.
+    // The clock is referenced when new timestamps are generated to ensure it produces monotonically increasing
+    // timestamps.
+    temporal.server.api.clock.v1.HybridLogicalClock clock = 1;


oh right. when I read reading above I was wondering where it stores the last used timestamp to avoid duplicates, but I didn't connect this to that. to be nitpicky: the comment above should say it's the last recorded cluster-local HLC timestamp for this task queue. another task queue might have a later one recorded (due to clock skew) but we never compare them across task queues so it doesn't matter.

dnr · 2023-04-19T06:23:02Z

schema/cassandra/temporal/schema.cql

+  data            binary, -- temporal.server.api.persistence.v1.TaskQueueUserData
+  data_encoding   text,   -- Encoding type used for serialization, in practice this should always be proto3
+  version         int64,  -- Version of this row, used for optimistic concurrency
+  -- task_queue_name is not a part of the parititioning key to allow iterating all task queues in a single namespace.


nitpick: even if it was part of the partition key, we could still iterate. it would just be more expensive

Suggested change

-- task_queue_name is not a part of the parititioning key to allow iterating all task queues in a single namespace.

-- task_queue_name is not a part of the parititioning key to allow cheaply iterating all task queues in a single namespace.

dnr · 2023-04-19T06:24:55Z

proto/internal/temporal/server/api/clock/v1/message.proto

+message HybridLogicalClock {
+    // Wall clock - A single time source MUST guarantee that 2 consecutive timestamps are monotonically non-decreasing.
+    // e.g. by storing the last wall clock and returning max(gettimeofday(), lastWallClock).
+    int64 wall_clock = 1;


do you want to document the units and epoch or does it not matter?

bergundy · 2023-04-20T05:50:06Z

Addressed all outstanding comments and also made a few schema changes after running integration tests and finding a few flaws.

bergundy · 2023-04-20T05:52:47Z

I'm merging this.
@dnr you may want to take a look at the sql tables, those changed a bit to use VARCHAR(255) for task_queue_name instead of task_queue_id in the previous commit.
The other matching tables use task_queue_id which is a combination of namespace ID, task queue name, and task queue type but I figured it's better to have separate columns for namespace and task queue.
If there are any comments I'll address in the next PR.

Note: This commit came from a feature branch and is not expected to build.

…io#4172) Note: This commit came from a feature branch and is not expected to build.

Note: This commit came from a feature branch and is not expected to build.

bergundy requested a review from a team as a code owner April 14, 2023 21:54

bergundy self-assigned this Apr 14, 2023

bergundy commented Apr 14, 2023

View reviewed changes

Sushisource reviewed Apr 14, 2023

View reviewed changes

bergundy and others added 4 commits April 17, 2023 16:37

Proto and persistence definitions for replicated versioning

f0af4e1

Use git refs for API and SDK repos, update api submodule to worker-ve…

f3ae6e0

…rsioning branch

Apply suggestions from code review

497fd14

Co-authored-by: Spencer Judge <sjudge@hey.com>

Update TaskQueueUserData comments

a96b861

bergundy force-pushed the task-queue-user-data branch from 7bcf918 to a96b861 Compare April 17, 2023 23:37

dnr reviewed Apr 18, 2023

View reviewed changes

Sushisource approved these changes Apr 18, 2023

View reviewed changes

Address review comments

0b71548

bergundy force-pushed the task-queue-user-data branch from d05d728 to 0b71548 Compare April 18, 2023 17:00

dnr approved these changes Apr 19, 2023

View reviewed changes

Address another round of review comments, fix schemas after some tests

c8d2923

bergundy merged commit 57b6718 into temporalio:worker-versioning Apr 20, 2023
4 of 5 checks passed

bergundy deleted the task-queue-user-data branch April 20, 2023 05:53

bergundy added a commit that referenced this pull request Apr 21, 2023

Proto and persistence definitions for replicated versioning (#4172)

6752299

bergundy added a commit that referenced this pull request Apr 25, 2023

Proto and persistence definitions for replicated versioning (#4172)

fdaca50

bergundy added a commit that referenced this pull request May 3, 2023

Proto and persistence definitions for replicated versioning (#4172)

26a8b0c

dnr pushed a commit that referenced this pull request May 11, 2023

Proto and persistence definitions for replicated versioning (#4172)

d223c8b

dnr pushed a commit that referenced this pull request May 26, 2023

Proto and persistence definitions for replicated versioning (#4172)

5217e3b

Note: This commit came from a feature branch and is not expected to build.

dnr pushed a commit to dnr/temporal that referenced this pull request May 26, 2023

Proto and persistence definitions for replicated versioning (temporal…

50fc4bb

…io#4172) Note: This commit came from a feature branch and is not expected to build.

dnr pushed a commit that referenced this pull request May 26, 2023

Proto and persistence definitions for replicated versioning (#4172)

b8b4b1a

Note: This commit came from a feature branch and is not expected to build.

dnr pushed a commit that referenced this pull request May 26, 2023

Proto and persistence definitions for replicated versioning (#4172)

c2fdc9f

Note: This commit came from a feature branch and is not expected to build.

	-- task_queue_name is not a part of the parititioning key to allow iterating all task queues in a single namespace.
	-- task_queue_name is not a part of the parititioning key to allow cheaply iterating all task queues in a single namespace.

Proto and persistence definitions for replicated versioning #4172

Proto and persistence definitions for replicated versioning #4172

Conversation

bergundy commented Apr 14, 2023

Choose a reason for hiding this comment

bergundy commented Apr 14, 2023

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

dnr 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

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

bergundy commented Apr 18, 2023

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

bergundy commented Apr 20, 2023

bergundy commented Apr 20, 2023