Split history tree insert from history node append #4147
Conversation
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = m.persistence.AppendHistoryNodes(ctx, req) | ||
| err = m.persistence.AppendHistoryNodes(ctx, nodeReq) |
There was a problem hiding this comment.
Append and Insert used to be in the same batch. Are you sure this won't cause any problems with them being in separate batches? For example, what happens if we crash after 536 and before 542?
There was a problem hiding this comment.
This failure condition (with nodes being written first with tree possibly not) is well known. The periodic scavenger task cleans these up.
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = m.persistence.AppendHistoryNodes(ctx, req) | ||
| err = m.persistence.AppendHistoryNodes(ctx, nodeReq) | ||
| if err == nil && request.IsNewBranch { |
There was a problem hiding this comment.
Why aren't we using the if err != nil { return err } pattern here?
There was a problem hiding this comment.
It did strike me as a bit odd why the size is returned regardless of error or not. But I believe that's why the original code also had this slight anti-pattern for error handling. I tracked this entire callstack chain and as far as I can tell, the size is expected. Hence, I'm following suit.
There was a problem hiding this comment.
I don't think we should repeat this pattern here because it sticks out like a sore thumb.
What changed?
Why?
How did you test it?
Potential risks
Is hotfix candidate?