Mutiple Bugfixes #794
Mutiple Bugfixes #794
Conversation
* fix workflow timeout version check bug * fix task event ID check bug * fix integtest race condition
service/history/conflictResolver.go
Outdated
| history, nextPageToken, err = r.getHistory(domainID, execution, common.FirstEventID, replayNextEventID, | ||
|
|
||
| var lastFirstEventID int64 | ||
| for remainingHistorySize := replayNextEventID - common.FirstEventID; remainingHistorySize > 0; { |
There was a problem hiding this comment.
why can't we rely on the nextPageToken? Are you worried about reading more events? I thought since we are going to bound the query using nextEventID, there is no way we can read more events than nextEventID. There is a chance we can read less events if the nextEventID falls in between a batch.
There was a problem hiding this comment.
- i did not change the query
- the change here make sure we are actually applying events up to the next event id. if the next event id < the highest event id in a batch of history
say in a batch, the first event id is 10, next event id is 15, and we only want events up to 13 (exclusive).
then this change can make it work, while before, we are actually applying all events from 10 -> 15
There was a problem hiding this comment.
My question is why can't we rely on nextPageToken for the looping condition. I agree with you that last batch could have more events within the batch after nextEventID (although it is not possible unless we have a bug somewhere else). But then it guarantees that there won't be any more batches after that. So it is still ok to loop on the nextPageToken logic.
|
|
||
| // load mutable state, if mutable state's next event ID <= task ID, will attempt to refresh | ||
| // if still mutable state's next event ID <= task ID, will return nil, nil | ||
| func loadMutableStateForTransferTask(context *workflowExecutionContext, transferTask *persistence.TransferTaskInfo, metricsClient metrics.Client, logger bark.Logger) (*mutableStateBuilder, error) { |
There was a problem hiding this comment.
Why only transfer task? Won't TimerTask have a similar issue?
There was a problem hiding this comment.
Is there a way to generalize the check for both timer and transfer tasks? Can we instead rely on version on the task and version on the mutable state?
There was a problem hiding this comment.
the main difference is here
isDecisionRetry := transferTask.TaskType == persistence.TransferTaskTypeDecisionTask &&
vs
isDecisionRetry := timerTask.TaskType == persistence.TaskTypeDecisionTimeout &&
service/history/failoverCheck.go
Outdated
| msBuilder.executionInfo.DecisionAttempt > 0 | ||
|
|
||
| if transferTask.ScheduleID >= msBuilder.GetNextEventID() && !isDecisionRetry { | ||
| metricsClient.IncCounter(metrics.TimerQueueProcessorScope, metrics.StaleMutableStateCounter) |
There was a problem hiding this comment.
Probably you want to use a different metric scope here.
wxing1292
force-pushed
the
bugfix-conflict
branch
from
2d8e5f2 to
898a060
Compare
solve #779 #770