Skip to content

Conversation

Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented Mar 27, 2025

What changed?

  • Replace GetReplicationTasks query with GetHistoryTasks query
  • Change the response of GetReplicationDLQTasks to GetHistoryTasksResponse
  • Update history replication handlers

Why?
To unify history task query

How did you test it?
unit tests, integration tests, dev2 tests (TBD)

Potential risks

Release notes

Documentation Changes

@@ -92,3 +88,45 @@ func (e *historyEngineImpl) GetDLQReplicationMessages(

return tasks, nil
}

func convertToReplicationTask(taskInfo *types.ReplicationTaskInfo) (persistence.Task, error) {
Copy link
Member Author

@Shaddoll Shaddoll Mar 27, 2025

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.

@Shaddoll Shaddoll force-pushed the p8 branch 2 times, most recently from cfd5e87 to 5a1969d Compare March 27, 2025 22:22
@Shaddoll Shaddoll changed the title wip: Unify history task query - part 5 - delete GetReplicationTasks method Unify history task query - part 5 - replication task queries Mar 27, 2025
@@ -112,5 +112,5 @@ func hydrateReplicationTask(
history.Find(info.NewRunBranchToken, constants.FirstEventID),
)

return hydrator.Hydrate(context.Background(), info)
return hydrator.Hydrate(context.Background(), task)
Copy link
Member

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?

Copy link
Member Author

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()
Copy link
Member

@taylanisikdemir taylanisikdemir Mar 28, 2025

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true.

@Shaddoll Shaddoll merged commit 53e9cb6 into cadence-workflow:master Mar 28, 2025
22 checks passed
@Shaddoll Shaddoll deleted the p8 branch March 28, 2025 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants