Assign LastFirstTransactionID correctly #2438
Conversation
Ardagan
force-pushed
the
LastFirstTransactionId
branch
from
4fb9373 to
9dd2ff6
Compare
| @@ -446,7 +446,7 @@ func (m *executionManagerImpl) ReadHistoryBranchByBatch( | |||
|
|
|||
| resp := &ReadHistoryBranchByBatchResponse{} | |||
| var err error | |||
| _, resp.History, resp.NextPageToken, resp.Size, err = m.readHistoryBranch(true, request) | |||
| _, resp.History, resp.TransactionIDs, resp.NextPageToken, resp.Size, err = m.readHistoryBranch(true, request) | |||
There was a problem hiding this comment.
This is getting out of hand. I think we need a struct for the results
There was a problem hiding this comment.
This is internal method used in two places only. I think looking at signature is faster for reading atm. My another concern would be that we create another "ReadHistoryBranch..." structure in code that will add even more ambiguity in naming than we have now.
| @@ -180,6 +187,8 @@ func (r *nDCStateRebuilderImpl) rebuild( | |||
| return nil, 0, err | |||
| } | |||
|
|
|||
| rebuiltMutableState.GetExecutionInfo().LastFirstEventTxnId = lastTxnId | |||
There was a problem hiding this comment.
we need a setter method on mutable state to set this
There was a problem hiding this comment.
the last txn ID should be managed by history builder, not here
There was a problem hiding this comment.
Synced with Wenquan:
- use current approach overall for this PR for quick fix and unblocking of GetWorkflowExecutionHistoryReverse feature
- followup with change to move managing of transaction ids to history_builder. Which would effectively include:
- Pass in event batch metadata to history_builder/rebuilder/etc when we apply/replay events
- Move transaction id generation out of mutable stage
- Make history_builder responsible for managing transaction ids
| // History events by batch | ||
| History []*historypb.History | ||
| // TransactionID for relevant History batch | ||
| TransactionIDs []int64 |
There was a problem hiding this comment.
how about changing
History []*historypb.History
TransactionIDs []int64to something like
History []Historywhere
History struct {
TransactionID int64
PrevTransactionID int64
Events *historypb.History
}There was a problem hiding this comment.
basically do not use 2 slices with equal length, but one slice of structs containing all data to be used
There was a problem hiding this comment.
I would suggest to make it a separate PR. It blows up into too big of a change.
What changed?
Assign LastFirstTxnID correctly for multiple branch case (reset workflow operation).
Why?
Establish expected behavior.
How did you test it?
unit tests
Potential risks
Reset workflow doesn't perform correctly
Is hotfix candidate?
No