Update Cassandra to write history tasks to new columns #6685
Conversation
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Shaddoll
force-pushed
the
persistence
branch
4 times, most recently
from
a0ced83 to
4ff2b56
Compare
Shaddoll
changed the title
Shaddoll
force-pushed
the
persistence
branch
2 times, most recently
from
ced5620 to
8f00caf
Compare
| tasks = append(tasks, t) | ||
| var blob *persistence.DataBlob | ||
| if d.dc.EnableShardIDMetrics() { | ||
| data, err := d.taskSerializer.SerializeTask(persistence.HistoryTaskCategoryTransfer, task) |
There was a problem hiding this comment.
As a follow up, we can consider cherry picking which fields are being serialized here to avoid redundancy. e.g. domainid/wfid/runid are available in the same row so they can be omitted.
There was a problem hiding this comment.
they cannot be omitted, because they are not stored in the primary key
| DomainID: executionInfo.DomainID, | ||
| TaskList: decision.TaskList, | ||
| ScheduleID: decision.ScheduleID, | ||
| TargetDomainID: executionInfo.DomainID, |
There was a problem hiding this comment.
Does this rename impact deserialization of already saved tasks on sql stores?
There was a problem hiding this comment.
And we probably don't need this as a separate field anymore since you have introduced WorkflowIdentifier. Could this be removed?
There was a problem hiding this comment.
it doesn't impact deserialization. It's the in-memory representation of history tasks.
I think it could be probably be removed and we can use domainID because activity and decisions cannot be scheduled in another domain. But I'll avoid that change in this PR and review it later.
| @@ -2037,6 +2037,8 @@ const ( | |||
| // Allowed filters: DomainName,TasklistName,TasklistType | |||
| MatchingEnableClientAutoConfig | |||
|
|
|||
| EnableNoSQLHistoryTaskDualWriteMode | |||
There was a problem hiding this comment.
Nit: it's not super clear from the name of this that this is referring to a migration of row encoding. I'd suggest either adding some detail to the name about that or at least a comment indicating it's intent. Otherwise it seems a lot more general than what appears to be a one-time migration.
|
|
||
| testCases := []struct { | ||
| category persistence.HistoryTaskCategory | ||
| task persistence.Task |
There was a problem hiding this comment.
I would suggest generating task data so that new fields added will be caught by the test if there's a mapping problem in your round-trip tests. see common/testing/testdatagen if it's useful.
What changed?
Why?
This is a followup change of #6671. We want to migrate Cassandra's transfer, timer, replication task data from typed columns to encoded blob columns introduced in that PR. After this change Cassandra will be in "dual write" mode.
Another followup PR will be created to allow specifying which column we read data from. Currently, we still read history tasks from the original columns as before.
No backfill is required for transfer tasks and replication tasks because old data not inserted with "dual write" mode will be gone soon after the deployment of this PR. A backfill process is required for timer tasks.
How did you test it?
Unit test, manual tests in dev2 environment, persistence test will be created in the followup PR that enables migration on reading tasks
Potential risks
tasks not encoded correctly (no impact production until we enable migration on reading tasks), history task processing might be broken
Release notes
Documentation Changes