Refactor tqname package #5329

ShahabT · 2024-01-22T04:05:22Z

What changed?

Replaced package tqname with tqid containing the following types
- TaskQueueFamily struct including namespace ID and user-given TQ name
- TaskQueue struct representing a high level task queue for a particular task type (activity vs workflow)
- Partition interface which can be either a NormalPartition or a StickyPartition
Replaced taskQueueID with PhysicalTaskQueueKey to represent a DB-level task queue
Delete stickyInfo
Replaced all usage of tqname and taskQueueID with new types depending on the exact entity that we are dealing with (TQ vs Sticky TQ partition vs normal TQ partition vs physical queue)
Created userDataManager responsible for fetching/loading and updating user data. Extracted all the related logic from taskQueueDB and taskQueuePartitionManger into userDataManager.
Renamed taskQueueManager to physicalTaskQueueManager

Why?

This is an overdue refactoring, we need proper distinction between all different entities mentioned above that are called "task queue".

How did you test it?

Mainly existing unit tests. Added few new ones where needed.

Potential risks

No known behavioral change is introduced, but need to keep an eye on this to make sure the refactoring has not change anything unintentionally.

Is hotfix candidate?

No.

tdeebswihart · 2024-01-23T21:38:06Z

common/taskqueue/task_queue.go

+type (
+
+	// TaskQueue can be NormalTaskQueue or StickyTaskQueue.
+	TaskQueue interface {


I'd love if we could avoid repetitive names like taskqueue.TaskQueue, what about names like taskqueue.Interface with implementations taskqueue.Normal and taskqueue.Sticky? You can see discussion of this on the official go blog:

Avoid repetition. Since client code uses the package name as a prefix when referring to the package contents, the names for those contents need not repeat the package name. The HTTP server provided by the http package is called Server, not HTTPServer. Client code refers to this type as http.Server, so there is no ambiguity.

Great feedback @tdeebswihart. I'm going to rename the types as you suggested, except I think I like to keep taskqueue.TaskQueue as is, renaming to taskqueue.Interface feels a little awkward to me. pkg.Pkg is mentioned in the link you referred and not discouraged. Many packages use that naming, e.g. time.Time.

dnr · 2024-01-23T22:46:18Z

There's a bunch of unrelated stuff on this pr so it'll take a while, or should be split. First just high level comments:

Replaced package tqname with taskqueue containing the following types

The name was chosen deliberately since the package does only name mangling. It should stay that way, and possibly additional functionality should be in another package (I'll have to see the details when I get into the code).

Also, we shouldn't name a package taskqueue.. that's already way overloaded.

Created userDataManager responsible for fetching/loading and updating user data. Extracted all the related logic from taskQueueDB and taskQueuePartitionManger into userDataManager.

This at least should be in a separate PR.

dnr

This is a partial review, everything outside of service/matching

client/matching/client.go

dnr · 2024-01-31T04:59:22Z

client/matching/metric_client.go

 		if err != nil {
+			c.logger.Info("invalid tq name "+taskQueue.String(), tag.Error(err))


use a tag for the name instead of string concatenation

dnr · 2024-01-31T05:01:54Z

cmd/tools/rpcwrappers/main.go

-		return fmt.Sprintf("client, err := c.getClientForTaskqueue(%s, %s, %s)", nsID.path, tq.path, tqt.path)
+
+		return fmt.Sprintf(
+			`tqPrtn, err := tqid.FromProto(%s, %s, %s)


tq is an okay abbreviation but maybe spell out Partition? or I don't even mind p here actually, just "Prtn" looks weird

dnr · 2024-01-31T05:03:12Z

common/tqid/task_queue_id.go

+	NonRootPartitionPrefix = "/_sys/"
+	PartitionDelimiter     = "/"


Can we keep these private? That's the whole point of consolidating the manipulation in one package, and it took quite a bit of effort. I don't like seeing that reverting.

dnr · 2024-01-31T05:03:45Z

common/tqid/task_queue_id.go

+)
+
+type (
+	// TaskQueue represents the high-level task queue that user create by explicitly providing a TaskQueue name


Suggested change

// TaskQueue represents the high-level task queue that user create by explicitly providing a TaskQueue name

// TaskQueue represents the high-level task queue that user creates by explicitly providing a TaskQueue name

dnr · 2024-01-31T06:50:48Z

service/history/workflow/workflow_task_state_machine.go

 	}
-	name, err := tqname.Parse(tq.Name)
+	prtn, err := tqid.FromProto(proto, "", 0)


same here with an enum.. I don't like these unused fields though :(

dnr · 2024-01-31T06:51:17Z

service/history/workflow/workflow_task_state_machine.go

 	}
-	name, err := tqname.Parse(tq.Name)
+	prtn, err := tqid.FromProto(proto, "", 0)


spell out partition

dnr · 2024-01-31T06:59:47Z

tests/versioning.go

@@ -66,7 +66,7 @@ type VersioningIntegSuite struct {

 const (
 	partitionTreeDegree = 3
-	longPollTime        = 5 * time.Second
+	longPollTime        = 10 * time.Second


does this make the test take longer? it's already pretty long to run this whole suite locally and I wouldn't want it to get any longer

dnr · 2024-01-31T07:01:26Z

tests/versioning.go

 			s.NoError(err)
-			partName = partName.WithPartition(pt.part)
+			partition := taskQueue.NormalPartition(0, pt.part)


shouldn't you pass pt.tp in here? if the partition now holds the type, it should hold the correct type

dnr · 2024-01-31T07:02:44Z

tests/versioning.go

 			// Use lower-level GetTaskQueueUserData instead of GetWorkerBuildIdCompatibility
 			// here so that we can target activity queues.
 			res, err := s.testCluster.host.matchingClient.GetTaskQueueUserData(
 				ctx,
 				&matchingservice.GetTaskQueueUserDataRequest{
 					NamespaceId:   nsId,
-					TaskQueue:     partName.FullName(),
+					TaskQueue:     partition.RpcName(),
 					TaskQueueType: pt.tp,


and then you can use partition.TaskType() here? (except please rename that to TaskQueueType)

…or-tqname

ast2023 · 2024-02-17T00:51:38Z

common/tqid/task_queue_id.go

+
+// FromFamilyName takes a user-provided task queue name (aka family name) and returns a TaskQueueFamily. Returns an
+// error if name looks like a mangled name.
+func FromFamilyName(namespaceId string, name string) (*TaskQueueFamily, error) {


We should incorporate TaskQueueFamily into function name. Unless namespace will be named TaskQueueFamily. When I read code tqid.FromFamilyName there is no way I can understand that we create a new TaskQueueFamily.

ast2023 · 2024-02-17T00:53:00Z

common/tqid/task_queue_id.go

+	return &TaskQueueFamily{namespace.ID(namespaceId), name}
+}
+
+func FromProto(proto *taskqueuepb.TaskQueue, namespaceId string, taskType enumspb.TaskQueueType) (Partition, error) {


Same as with TaskQueueFamily. We should incorporate Partition into function name. It sould be PartitionFromProto or something like this.

ast2023 · 2024-02-17T00:55:57Z

common/tqid/task_queue_id.go

+	return n.name
+}
+
+func (n *TaskQueueFamily) NamespaceID() namespace.ID {


Do we have agreement on proper case for id? Id is a short of Identification. It is not abbreviation. So I vote for Id (capital I lower case d)

Good point. I'm also not sure what is the convention here. It seems both styles are used in different places of the code. I also like NamespaceId better. So changing it according to your suggestion.

Alex agreed that ID was a mistake. We are shifting to Id where possible.

dnr

I got through a big chunk of the rest, but not quite all of it. sending comments-so-far

dnr · 2024-02-21T08:25:11Z

client/matching/client.go

-		return nil, err
+		// We preserve the old logic (not returning error in case of invalid proto info) until it's verified that
+		// clients are not sending invalid names.
+		c.logger.Info("invalid tq partition", tag.Error(err), tag.NewStringsTag("proto", []string{proto.String()}))


why strings?

Suggested change

c.logger.Info("invalid tq partition", tag.Error(err), tag.NewStringsTag("proto", []string{proto.String()}))

c.logger.Info("invalid tq partition", tag.Error(err), tag.NewStringTag("proto", proto.String()))

dnr · 2024-02-21T08:27:41Z

common/persistence/cassandra/execution_store.go

@@ -36,7 +36,7 @@ import (
 // Guidelines for creating new special UUID constants
 // Each UUID should be of the form: E0000000-R000-f000-f000-00000000000x
 // Where x is any hexadecimal value, E represents the entity type valid values are:
-// E = {NamespaceID = 1, WorkflowID = 2, RunID = 3}
+// E = {NamespaceId = 1, WorkflowID = 2, RunID = 3}


ok but weird to have this change in this PR?

Sometimes my IDE rename things that it should not touch :/

dnr · 2024-02-21T08:28:18Z

common/tqid/task_queue_id.go

+	// TaskQueueFamily represents the high-level "task queue" that user creates by explicitly providing a task queue name
+	// when starting a worker or a workflow. A task queue family consists of separate TaskQueue's for different types of
+	// task.


Suggested change

// TaskQueueFamily represents the high-level "task queue" that user creates by explicitly providing a task queue name

// when starting a worker or a workflow. A task queue family consists of separate TaskQueue's for different types of

// task.

// TaskQueueFamily represents the high-level "task queue" that user creates by explicitly providing a task queue name

// when starting a worker or a workflow. A task queue family consists of separate TaskQueues for different types of

// task (e.g. Workflow, Activity).

dnr · 2024-02-21T08:28:58Z

common/tqid/task_queue_id.go

+	// TaskQueue represents a logical task queue for a type of tasks (e.g. Activity or Workflow). Under the hood,
+	// a TaskQueue can be broken down to multiple sticky or normal partitions.
+	TaskQueue struct {
+		family   TaskQueueFamily


consider embedding TaskQueueFamily directly? does that make any code simpler?

I did consider that, but decided not to do it because a task queue is not a family. some methods in family do not make sense to be added to TaskQueue.

dnr · 2024-02-21T08:30:37Z

common/tqid/task_queue_id.go

 	}

 	switch kind {
 	case enumspb.TASK_QUEUE_KIND_STICKY:
 		if partition != 0 {
 			return nil, fmt.Errorf("%w. base name: %s, normal name: %s", ErrNonZeroSticky, baseName, normalName)
 		}
-		tq := &TaskQueue{namespace.ID(namespaceId), normalName}
+		tq := &TaskQueue{TaskQueueFamily{namespace.ID(namespaceId), normalName}, taskType}


I'm surprised the linter doesn't complain about using positional args for struct initialization...

I'd name the fields, but also if you embed TaskQueueFamily it's a little nicer

some IDEs fill the names automatically:

I'd name them if the number of arguments was more, but personally I prefer this way in short cases.

dnr · 2024-02-21T08:57:27Z

service/matching/db_task_queue.go

+	}
+
+	// unversioned DB queues use the RPC name of their partition
+	return dbq.partition.RpcName()


the whole point of copying those prefix+delimiter strings into this package is to decouple the rpc naming from persistence key naming (even though they're identical for unversioned). so just construct the string with this package's delimiters here, don't rely on RpcName

dnr · 2024-02-21T08:59:10Z

service/matching/db_task_queue.go

+//
+// All versioned DB queues use mangled names, using the following format:
+//
+//	with build ID: 		/_sys/<base name>/<build ID base64 URL encoded>;<partition id>


Suggested change

// with build ID: /_sys/<base name>/<build ID base64 URL encoded>;<partition id>

// with build ID: /_sys/<base name>/<build ID base64 URL encoded>#<partition id>

dnr · 2024-02-21T09:16:43Z

service/matching/matching_engine.go

 	if err != nil {
 		return nil, err
 	}
-	tqMgr, err := e.getTaskQueuePartitionManager(ctx, taskQueue, normalStickyInfo, true)
+	tqMgr, err := e.getTaskQueuePartitionManager(ctx, taskQueueFamily.TaskQueue(enumspb.TASK_QUEUE_TYPE_WORKFLOW).RootPartition(), true)


it looks weird to be just assuming workflow+root here.. I see it's correct because of how we route this in client, but I think a comment would be nice to assure readers

dnr · 2024-02-21T09:20:36Z

service/matching/matching_engine.go

 	if err != nil {
 		return nil, err
 	}
-	tqm, err := e.getTaskQueuePartitionManager(ctx, taskQueue, normalStickyInfo, false)
+	pm, err := e.getTaskQueuePartitionManager(ctx, taskQueueFamily.TaskQueue(req.TaskQueueType).RootPartition(), true)


this one seems wrong, though.. why are we assuming the root here? shouldn't this work on any partition?

good catch. fixed.

dnr · 2024-02-21T09:23:25Z

service/matching/task_writer.go

@@ -65,9 +65,9 @@ type (
 	// taskWriter writes tasks sequentially to persistence
 	taskWriter struct {
 		status       int32
-		tlMgr        *taskQueueManagerImpl
+		tlMgr        *dbQueueManagerImpl


no need to change it, I just wanted to say that I'm amused that we're changing the type but keep the tl in the name from pre-temporal :)

dnr · 2024-02-22T08:26:17Z