-
Notifications
You must be signed in to change notification settings - Fork 877
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
Add activity retry policy properties to replication #7055
Conversation
@@ -155,6 +155,10 @@ func (r *ActivityStateReplicatorImpl) SyncActivityState( | |||
LastAttemptCompleteTime: request.LastAttemptCompleteTime, | |||
Stamp: request.Stamp, | |||
Paused: request.Paused, | |||
RetryInitialInterval: request.RetryInitialInterval, |
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.
also need it for SyncActivitiesState?
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.
Not sure I understand this comment. Any action required?
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.
we don't need to change SyncActivitiesState. historyservice.SyncActivitiesRequest
is using ActivitySyncInfo
so not extra conversion is needed.
historyservice.SyncActivityRequest
though explicitly lists all the fields so need this extra conversion.
@@ -155,6 +155,10 @@ func (r *ActivityStateReplicatorImpl) SyncActivityState( | |||
LastAttemptCompleteTime: request.LastAttemptCompleteTime, | |||
Stamp: request.Stamp, | |||
Paused: request.Paused, | |||
RetryInitialInterval: request.RetryInitialInterval, |
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.
we don't need to change SyncActivitiesState. historyservice.SyncActivitiesRequest
is using ActivitySyncInfo
so not extra conversion is needed.
historyservice.SyncActivityRequest
though explicitly lists all the fields so need this extra conversion.
Do we need to change the SyncActivityRequest here? |
Do we want to support new features in the old rpc based replication stack? |
// stamp changed, should update activity | ||
return true | ||
} | ||
|
||
if activityInfo.Stamp > stamp { | ||
// stamp changed, should update activity |
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.
nit: fix the comment.
What changed?
Add the following activity retry policy properties to the activity replication logic:
Why?
Before there was assumption that activity retry policy can't change. Because of that it was not replicated.
Now we can change activity retry policy via UpdateActivity API. Those changes should be replicated.
How did you test it?
Add func tests that check few updated properties.
Potential risks
Increasing replication size.
Is hotfix candidate?
No