Refactoring Cassandra workflow persistence manager for NoSQL support : Part 1 #4251
Conversation
github-actions
bot
force-pushed
the
qlong-nosql-wf-1
branch
from
6b32c78
to
e9dd02e
Compare
longquanzheng
force-pushed
the
qlong-nosql-wf-1
branch
from
e9dd02e
to
bacbbd7
Compare
longquanzheng
force-pushed
the
qlong-nosql-wf-1
branch
from
d481050
to
9b34f61
Compare
| * cross_cluster_task is to store also background tasks that need to be processed right after the transaction, and only for | ||
| * but only for CrossDC(XDC) replication feature. Each record is a replication task generated for a target cluster. | ||
| * CrossCluster task stores information similar to TransferTask. |
There was a problem hiding this comment.
There's some mismatch here?
There was a problem hiding this comment.
Which part? I probably don't understand it well. Do you mean it's not XDC or not replication? Feel free to correct it.
There was a problem hiding this comment.
I think it is "and only for but only for CrossDC(XDC) replication feature" under cross cluster task. I think it is just cross cluster execution feature. What do you think? @yycptt
| signalInfoMap map[int64]*persistence.SignalInfo, | ||
| signalRequestedIDs []string, | ||
| shardCondition *ShardCondition, | ||
| ) (*ConditionFailureReason, error) |
There was a problem hiding this comment.
shall we define/reuse some (existing) error types for those failure cause, and return them as part of error?
There was a problem hiding this comment.
So I kinda invented this way for strongly typed experience. It's also in shardCRUD and taskCRUD:
The benefits is that we don't need to do type any casting from error interface into a struct.
There was a problem hiding this comment.
I just did some research. Seems like using error struct is the standard way in Golang. I will change that here. Also created #4273 to change taskCRUD and shardCRUD.
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go
Outdated
Show resolved
Hide resolved
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go
Outdated
Show resolved
Hide resolved
longquanzheng
force-pushed
the
qlong-nosql-wf-1
branch
from
9b34f61
to
4b95143
Compare
| executionInfo, versionHistories, checkSum, | ||
| nowTimestamp, lastWriteVersion, |
| } | ||
|
|
||
| func (d *cassandraPersistence) prepareTimerTasksForWorkflowTxn( | ||
| domainID, workflowID, runID string, |
There was a problem hiding this comment.
I think they are all strings and quite related. This should read more fluently :P
LMK if you have a strong opinion on that. I actually did that intentionally.
| rowTypeShardTaskID, | ||
| request.RangeID, | ||
| ) | ||
| activityInfos, err := d.prepareActivityInfosForWorkflowTxn(newWorkflow.ActivityInfos) |
There was a problem hiding this comment.
Do we need this in create workflow operation?
There was a problem hiding this comment.
addressed offline. This is probably not being used but I have to keep the same behavior for refactoring.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| childWorkflowInfos, err := d.prepareChildWFInfosForWorkflowTxn(newWorkflow.ChildExecutionInfos) |
There was a problem hiding this comment.
Do we need this in create workflow operation?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| requestCancelInfoMap, err := d.prepareRequestCancelsForWorkflowTxn(newWorkflow.RequestCancelInfos) |
There was a problem hiding this comment.
Do we need this in create workflow operation?
| } | ||
| func (d *DataBlob) GetEncodingString() string { |
There was a problem hiding this comment.
sure, thanks!
| return err | ||
| } | ||
|
|
||
| err = db.updateActivityInfos(batch, shardID, domainID, workflowID, runID, activityInfoMap, nil) |
There was a problem hiding this comment.
Same. Not sure for insert do we need this?
| if err != nil { | ||
| return err | ||
| } | ||
| err = db.updateChildExecutionInfos(batch, shardID, domainID, workflowID, runID, childWorkflowInfoMap, nil) |
There was a problem hiding this comment.
Same. Not sure for insert do we need this?
| if err != nil { | ||
| return err | ||
| } | ||
| err = db.updateRequestCancelInfos(batch, shardID, domainID, workflowID, runID, requestCancelInfoMap, nil) |
There was a problem hiding this comment.
Same. Not sure for insert do we need this?
| * cross_cluster_task is to store also background tasks that need to be processed right after the transaction, and only for | ||
| * but only for CrossDC(XDC) replication feature. Each record is a replication task generated for a target cluster. | ||
| * CrossCluster task stores information similar to TransferTask. |
There was a problem hiding this comment.
I think it is "and only for but only for CrossDC(XDC) replication feature" under cross cluster task. I think it is just cross cluster execution feature. What do you think? @yycptt
longquanzheng
force-pushed
the
qlong-nosql-wf-1
branch
from
3964c20
to
384d88c
Compare
It's not being used at all. applyWorkflowSnapshotBatchAsNew is still being used by UpdateWorkflow and ConflictResolution API so they will be deleted in next PR. I don't feel it's useful at all to have a feature flag. The refactoring goal is to get rid of it. |
github-actions
bot
force-pushed
the
qlong-nosql-wf-1
branch
from
384d88c
to
0958819
Compare
What changed?
Why?
For #3514
How did you test it?
existing tests. Working on adding more test to the PR.
Potential risks
Medium risks.
Even though the goal of the PR is to keep the same behavior, it's still some uknown and this is the critical persistence for Cadence.
Release notes
No
Documentation Changes
No