Update C* queries about execution table with timestamps #6593
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
bowenxia
changed the title
| @@ -725,10 +728,10 @@ func (db *cdb) InsertReplicationTask(ctx context.Context, tasks []*nosqlplugin.R | |||
| shardID := shardCondition.ShardID | |||
| batch := db.session.NewBatch(gocql.LoggedBatch).WithContext(ctx) | |||
| for _, task := range tasks { | |||
| createReplicationTasks(batch, shardID, task.DomainID, task.WorkflowID, []*nosqlplugin.ReplicationTask{task}) | |||
| createReplicationTasks(batch, shardID, task.DomainID, task.WorkflowID, []*nosqlplugin.ReplicationTask{task}, time.Now()) | |||
There was a problem hiding this comment.
is the db.timesource available for these too?
There was a problem hiding this comment.
Yes. Update it.
But why is db.timesource.now() better than time.Now() ?
Codecov ReportAttention: Patch coverage is
... and 19 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -724,11 +727,12 @@ func (db *cdb) InsertReplicationTask(ctx context.Context, tasks []*nosqlplugin.R | |||
|
|
|||
| shardID := shardCondition.ShardID | |||
| batch := db.session.NewBatch(gocql.LoggedBatch).WithContext(ctx) | |||
| timeStamp := db.timeSrc.Now() | |||
There was a problem hiding this comment.
My point of view:
It is incorrect. The field belongs to the ReplicationTask, so an SQL plugin could later support it.
I don't think db should have timeSrc field, this should be filled on the PersistenceManager level and pushed as is to the DB.
There was a problem hiding this comment.
db object was using time.Now() in a few places since the beginning. While we were adding unit tests last year I replaced all time.Now()s with a db.timeSrc.Now() so the generated queries can be validated in the tests. This PR is following same pattern.
What you suggest is valid I think. The timestamp should be determined by the upper layer PersistentManager and the db is purely translating the entity records to DB queries without adding anything else on top. However that requires some refactoring. I don't think is is a source of bug but it's just better isolation of responsibilities. Let's turn that into a github issue and label with #up-for-grab.
bowenxia
deleted the
xbowen/update_cassandra_query_with_timestamps/execution_table
branch
What changed?
Update queries about execution table with created_time and last_updated_time
Updated unit test accordingly
Why?
to make rows in Cassandra easier to debug
How did you test it?
unit test
manually tested in mono repo
Potential risks
Release notes
Documentation Changes