-
Notifications
You must be signed in to change notification settings - Fork 854
Unify history task query - part 5 - replication task queries #6761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
65c5fdd
to
a22d831
Compare
@@ -92,3 +88,45 @@ func (e *historyEngineImpl) GetDLQReplicationMessages( | |||
|
|||
return tasks, nil | |||
} | |||
|
|||
func convertToReplicationTask(taskInfo *types.ReplicationTaskInfo) (persistence.Task, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function is created here not in types
package to avoid circular dependency.
cfd5e87
to
5a1969d
Compare
@@ -112,5 +112,5 @@ func hydrateReplicationTask( | |||
history.Find(info.NewRunBranchToken, constants.FirstEventID), | |||
) | |||
|
|||
return hydrator.Hydrate(context.Background(), info) | |||
return hydrator.Hydrate(context.Background(), task) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we even need info
object in this func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used by hydrator
@@ -173,7 +173,7 @@ func (m *MetricsEmitterImpl) determineReplicationLatency(remoteClusterName strin | |||
|
|||
var replicationLatency time.Duration | |||
if len(tasks) > 0 { | |||
creationTime := time.Unix(0, tasks[0].CreationTime) | |||
creationTime := tasks[0].GetVisibilityTimestamp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just double checking, does GetVisibilityTimestamp()
return CreationTime for replication tasks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true.
What changed?
Why?
To unify history task query
How did you test it?
unit tests, integration tests, dev2 tests (TBD)
Potential risks
Release notes
Documentation Changes