Add task list partition config #6343

Shaddoll · 2024-10-09T01:10:39Z

Detailed Description

Update Cassandra schema with new task_list_partition_config type and add the type as a new field to task_list type.
Pull the latest idl update.
Update persistence layer to store task_list_partition_config in database.

Impact Analysis

Backward Compatibility: It's backward compatible.
Forward Compatibility: It's forward compatible. Old binaries will ignore the new field introduced in this PR when reading data.

Testing Plan

Unit Tests: Yes
Persistence Tests: Yes
Integration Tests: Yes
Compatibility Tests: Backward/Forward compatibility has been checked on laptop.

Rollout Plan

What is the rollout plan? We need to update cassandra schema before rollout.
Does the order of deployment matter? No.
Is it safe to rollback? Does the order of rollback matter? It's safe to rollback.
Is there a kill switch to mitigate the impact immediately? No.

codecov · 2024-10-14T20:20:54Z

Codecov Report

Attention: Patch coverage is 93.71069% with 10 lines in your changes missing coverage. Please review.

Project coverage is 74.51%. Comparing base (a32159c) to head (7695348).
Report is 16 commits behind head on master.

Files with missing lines	Patch %	Lines
...n/persistence/nosql/nosqlplugin/cassandra/tasks.go	85.71%	2 Missing and 2 partials ⚠️
common/persistence/nosql/nosql_task_store.go	95.55%	1 Missing and 1 partial ⚠️
common/persistence/sql/sql_task_store.go	96.55%	1 Missing and 1 partial ⚠️
common/persistence/taskManager.go	0.00%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
common/persistence/data_manager_interfaces.go	`96.03% <ø> (ø)`
common/persistence/data_store_interfaces.go	`100.00% <ø> (ø)`
common/persistence/serialization/thrift_mapper.go	`89.64% <100.00%> (+0.28%)`	⬆️
...ommon/persistence/wrappers/errorinjectors/utils.go	`91.56% <100.00%> (+0.06%)`	⬆️
common/persistence/nosql/nosql_task_store.go	`72.97% <95.55%> (+1.90%)`	⬆️
common/persistence/sql/sql_task_store.go	`82.75% <96.55%> (+1.86%)`	⬆️
common/persistence/taskManager.go	`0.00% <0.00%> (ø)`
...n/persistence/nosql/nosqlplugin/cassandra/tasks.go	`97.79% <85.71%> (-1.40%)`	⬇️

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a32159c...7695348. Read the comment docs.

davidporter-id-au · 2024-10-16T01:32:49Z

schema/cassandra/cadence/schema.cql

 CREATE TYPE task_list (
  domain_id        uuid,
  name             text,
  type             int, -- enum TaskRowType {ActivityTask, DecisionTask}
  ack_level        bigint, -- task_id of the last acknowledged message
  kind             int, -- enum TaskListKind {Normal, Sticky}
-  last_updated     timestamp
+  last_updated     timestamp,
+  adaptive_partition_config frozen<task_list_partition_config>


the use of the word adaptive here precludes it for being used in other contexts. What do you think about just partition_config in case it isn't set to be adaptive for some reason?

I don't want to confuse this config with the partition_config for zonal isolation.

common/persistence/sql/sql_task_store.go

common/persistence/nosql/nosqlplugin/cassandra/tasks.go

davidporter-id-au · 2024-10-16T01:44:47Z

common/persistence/nosql/nosqlplugin/cassandra/tasks_test.go

@@ -66,6 +66,11 @@ func TestSelectTaskList(t *testing.T) {
 					(*tlDB)["ack_level"] = int64(1000)
 					(*tlDB)["kind"] = 2
 					(*tlDB)["last_updated"] = now
+					(*tlDB)["adaptive_partition_config"] = map[string]interface{}{


Nonblocking/Nit: It might be be worth doing a round-trip test on the to and from mapping functions to ensure that no fields get dropped in the future.

taylanisikdemir · 2024-10-18T05:10:25Z

schema/cassandra/cadence/schema.cql

 CREATE TYPE task_list (
  domain_id        uuid,
  name             text,
  type             int, -- enum TaskRowType {ActivityTask, DecisionTask}
  ack_level        bigint, -- task_id of the last acknowledged message
  kind             int, -- enum TaskListKind {Normal, Sticky}
-  last_updated     timestamp
+  last_updated     timestamp,
+  adaptive_partition_config frozen<task_list_partition_config>


I think we should make this a byte array representing a thrift serialized blob. This way further iterations on this adaptive_partition_config (e.g. adding zone info or other metadata) will not require DB schema change

My argument is if we want to use serialized blob, we should do it for task_list and task type. And that's what temporal does.

I think any future structs like this (no-need-to-query-fields) that we introduce going forward should be modeled as binary blob for flexibility purposes, therefore no more new DB types. Migrating away from existing DB types to blob approach is not P0 in my mind. We can consider a major schema at some point and that would the good time to get model such structs as blob.
However if you think extending this DB type with a few more fields in the near future is not going to be too much work (including the schema rollout) then let's go ahead with this.

taylanisikdemir · 2024-10-18T05:14:46Z

common/persistence/persistence-tests/matchingPersistenceTest.go

+	_, ok = err.(*p.ConditionFailedError)
+	s.True(ok)


these 3 lines can be replaces by s.ErrorAs

taylanisikdemir · 2024-10-18T18:14:21Z

schema/cassandra/cadence/schema.cql

@@ -260,13 +260,21 @@ CREATE TYPE task (
  partition_config map<text, text>
 );

+CREATE TYPE task_list_partition_config (


@natemort can you check this type and give feedback on what fields you would need for upcoming zonal isolation improvements?
It's better to avoid extra schema changes but if we don't know the shape of it then it's fine. We can come back and change this later.

Discussed with Zijian, this approach is fine and isn't incompatible. We'll make additional changes in the future since things are a little up in the air still.

Shaddoll requested review from neil-xie, davidporter-id-au, Groxx, shijiesheng, jakobht, 3vilhamster, sankari165, dkrotx, taylanisikdemir and demirkayaender as code owners October 9, 2024 01:10

Shaddoll force-pushed the schema branch 5 times, most recently from e87850b to 9fa4083 Compare October 14, 2024 19:43

Add task list partition config to schema

7695348

Shaddoll force-pushed the schema branch from 9fa4083 to 7695348 Compare October 14, 2024 19:47

Shaddoll changed the title ~~wip: Add task list partition config to schema~~ Add task list partition config Oct 14, 2024

davidporter-id-au reviewed Oct 16, 2024

View reviewed changes

common/persistence/sql/sql_task_store.go Show resolved Hide resolved

davidporter-id-au reviewed Oct 16, 2024

View reviewed changes

common/persistence/nosql/nosqlplugin/cassandra/tasks.go Show resolved Hide resolved

davidporter-id-au reviewed Oct 16, 2024

View reviewed changes

taylanisikdemir reviewed Oct 18, 2024

View reviewed changes

taylanisikdemir approved these changes Oct 18, 2024

View reviewed changes

Shaddoll merged commit 9b75b7d into uber:master Oct 18, 2024
21 checks passed

Shaddoll deleted the schema branch October 18, 2024 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add task list partition config #6343

Add task list partition config #6343

Shaddoll commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

davidporter-id-au Oct 16, 2024

Shaddoll Oct 16, 2024

davidporter-id-au Oct 16, 2024

taylanisikdemir Oct 18, 2024

Shaddoll Oct 18, 2024 •

edited

Loading

taylanisikdemir Oct 18, 2024 •

edited

Loading

taylanisikdemir Oct 18, 2024

taylanisikdemir Oct 18, 2024

natemort Oct 18, 2024

Add task list partition config #6343

Add task list partition config #6343

Conversation

Shaddoll commented Oct 9, 2024 • edited Loading

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

davidporter-id-au Oct 16, 2024

Choose a reason for hiding this comment

Shaddoll Oct 16, 2024

Choose a reason for hiding this comment

davidporter-id-au Oct 16, 2024

Choose a reason for hiding this comment

taylanisikdemir Oct 18, 2024

Choose a reason for hiding this comment

Shaddoll Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

taylanisikdemir Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

taylanisikdemir Oct 18, 2024

Choose a reason for hiding this comment

taylanisikdemir Oct 18, 2024

Choose a reason for hiding this comment

natemort Oct 18, 2024

Choose a reason for hiding this comment

Shaddoll commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

Shaddoll Oct 18, 2024 •

edited

Loading

taylanisikdemir Oct 18, 2024 •

edited

Loading