Improve msn comments#22339
Conversation
github-actions
Bot
added
area: runtime
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 6efb044 |
|
|
||
| // Intercept to reduce minimum sequence number to the delta manager's minimum sequence number. | ||
| // Sequence numbers are not guaranteed to follow any sort of order. Re-entrancy is one of those situations | ||
| // Minimum sequence numbers are not guaranteed to follow any sort of order. Re-entrancy is one of those situations |
There was a problem hiding this comment.
This comment is still misleading. It's incorrect to say that min seq# are not guaranteed to follow any sort of order. They do follow an order and are always increasing similar to sequence number.
The reason this is needed is because the runtime could be tracking ops in pending state manager whose min seq# is lower than delta manager's because delta manager knows nothing about these pending ops. Basically, container runtime's knowledge of min seq# can be different from delta managers. So, we need to modify the min seq# before sending these ops to the lower layers for correctness.
There was a problem hiding this comment.
Also, not sure what you mean by "Re-entrancy is one of those situations". This comment should expand on that. What scenario does re-entrancy create that would lead to updating the min seq # here.
There was a problem hiding this comment.
This is not guaranteed, one of the op re-entrancy tests breaks when I tried to introduce this invariant that minimum sequence number is always increasing.
There was a problem hiding this comment.
Update, I added an assert to validate the invariant, it seems to hold, I was wrong. Updating the PR
There was a problem hiding this comment.
Okay, good. Irrespective, the statement min squence numbers are not guaranteed to follow order is incorrect. The server will always stamp them in order. The client may have some scenarios where it changes the order while processing but the invariant still holds. So, even if you don't have the assert, I would remove that generic statement :)
There was a problem hiding this comment.
@markfields fyi - I know you might be working in this area, so it's best you're informed of this part of the runtime.
Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
4 participants
There is no task associated with this.
Relevant reference PR: #20856
@agarwal-navin and I had an offline conversation about this comment, and it was hard to understand. I've now improved the comment for it to be easier to understand.