-
Notifications
You must be signed in to change notification settings - Fork 805
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
Ut conflict resolver #806
Ut conflict resolver #806
Conversation
a728846
to
1f726bd
Compare
@@ -107,6 +108,9 @@ func (r *conflictResolver) reset(replayEventID int64, startTime time.Time) (*mut | |||
// the last updated time is not important here, since this should be updated with event time afterwards | |||
resetMutableStateBuilder.executionInfo.LastUpdatedTimestamp = startTime | |||
|
|||
sourceCluster := r.clusterMetadata.ClusterNameForFailoverVersion(resetMutableStateBuilder.GetCurrentVersion()) |
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.
I think you should read the version from last event rather than rely on CurrentVersion.
service/history/stateBuilder.go
Outdated
*decisionInfo, *mutableStateBuilder, error) { | ||
var lastEvent *shared.HistoryEvent | ||
var lastDecision *decisionInfo | ||
var newRunStateBuilder *mutableStateBuilder | ||
for _, event := range history.Events { | ||
lastEvent = event | ||
// must set the current version, since this is standby here, not active | ||
b.msBuilder.updateReplicationStateVersion(event.GetVersion()) |
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.
I'm not sure this is the right place to update CurrentStateVersion. It is currently updated on workflow execution load. I think we should just update the API on 'updateReplicationStateLastEventID' to also take in clusterName and lastWriteVersion and update both lastWriteVersion and lastWriteEventID as part of the same API. Then we can call this explicitly from conflict resolver at the end like we update other mutableState fields.
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.
Let's hold onto this till Monday. I think we should have a quick discussion before we land this.
func (e *mutableStateBuilder) updateReplicationStateLastEventID(clusterName string, lastEventID int64) { | ||
func (e *mutableStateBuilder) updateReplicationStateLastEventID(clusterName string, lastWriteVersion, | ||
lastEventID int64) { | ||
e.replicationState.LastWriteVersion = lastWriteVersion | ||
e.replicationState.LastWriteVersion = e.replicationState.CurrentVersion |
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 line should be deleted.
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.
and the active workflow, when calling this function, should use the current version
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.
fixed.
*bugfix: getHistory should return output next token, not input token
*bugfix: state builder should set the version on mutable state before apply events, especially when ms builder is reset.
partially solve #791