[WIP] Move time source from db layer to PersistenceManager #6646
Conversation
ribaraka
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -60,7 +60,7 @@ func TestNosqlExecutionStoreUtils(t *testing.T) { | |||
| Data: []byte(`[{"Branches":[{"BranchID":"test-branch-id","BeginNodeID":1,"EndNodeID":2}]}]`), | |||
| }, | |||
| } | |||
| return store.prepareCreateWorkflowExecutionRequestWithMaps(workflowSnapshot) | |||
| return store.prepareCreateWorkflowExecutionRequestWithMaps(workflowSnapshot, time.Now()) | |||
There was a problem hiding this comment.
Have you had any error when running the unit tests? I was thinking to use a constant time stamp in the tests like this so that in the unit tests it will always keep the same
There was a problem hiding this comment.
No, there haven't been any errors in the tests with this implementation. However, I refactored it to use a FixedTime variable for consistency.
ribaraka
force-pushed
the
mv-timeSrc-to-persistManager
branch
from
e99014a to
f1bb070
Compare
|
@bowenxia added another commit with a refactor of the history storage. If you have time, please take a look. I'll be sending the work in small chunks, little by little. There are a few TODOs— if you could reply to them, I’d really appreciate it. |
| @@ -54,19 +53,20 @@ func (db *cdb) InsertIntoHistoryTreeAndNode(ctx context.Context, treeRow *nosqlp | |||
| // Note: for perf, prefer using batch for inserting more than one records | |||
| batch := db.session.NewBatch(gocql.LoggedBatch).WithContext(ctx) | |||
| batch.Query(v2templateInsertTree, | |||
| treeRow.TreeID, treeRow.BranchID, ancs, persistence.UnixNanoToDBTimestamp(treeRow.CreateTimestamp.UnixNano()), treeRow.Info, timeStamp) | |||
| // TODO: Should the `persistence.UnixNanoToDBTimestamp(treeRow.CreateTimestamp.UnixNano())` logic be handled by `persistenceManager` as well, or should it remain as is? | |||
There was a problem hiding this comment.
Let's leave it as is, since this PR is more related to the created_time and last_updated_time timestamps.
|
LGTM so far. Lemme know if you are planning to refactor other persistence managers here, or in a new PR. |
I'm planning to do everything in one PR. I figured it would be more convenient to have the refactor in a single place rather than splitting it up, unless there are any objections. |
Refactored the system so that the PersistenceManager level handles the timestamp
Why?
#6610
How did you test it?
unit tests
Potential risks
Release notes
Documentation Changes