Skip to content
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

Test: Regression for non-ordered queue #826

Closed

Conversation

ypatil12
Copy link
Collaborator

@ypatil12 ypatil12 commented Oct 15, 2024

The question with the proposed bug is: Does deallocating and then allocating

  1. Mess up reads to allocated magnitude
  2. Mess up the modification queue.

Let's take the following example:

Allocation delay is 15 days. Assume same strategy is allocated to

t=0
Queue magnitude allocation for opset_1 to 50. The effectTimestamp is 15

t=15
Queue deallocation for opset_1 to 25.
The effectTimestamp is 32.5

t=15
Queue allocation for opset_2 to 33
The effectTimestamp is 30

Now the queue looks like

  1. dealloc, opset_1, mag: 25, timestamp: 32.5
  2. alloc, opset_2, mag: 33, timestamp: 30

Let's look at reads to allocated magnitude

  • getAllocatableMagnitude: When we allocate for opset_2, we increase the encumbered magnitude. Hence, allocatableMagnitude returns the proper value
  • modifyAllocations: at time t=30 we do not clear the modification queue for the deallocation & allocation. HOWEVER, when we call
    _getPendingMagnitudeInfo this basically "pseudo clears" that allocation. pendingDiff is set to 0 from this line of code since the effectTimestamp has been passed
  • slashOperator: at t=30, this magnitude is still in effect due to _getPendingMagnitudeInfo

Let's look at writes to completeModificationQueue

It is true, that the deallocation is BLOCKING the allocation. However, given the above, it does not mess up any reads to state since _getPendingMagnitudeInfo does a lot of heavy lifting
The edge case we actually need to solve for is if an allocation blocks a deallocation.

The worst case scenario is if we had something like

  1. dealloc, opset_1, mag: 25, timestamp: 32.5
  2. alloc, opset_2, mag: 33, timestamp: 40
  3. dealloc, opset_3, mag: 10, timestamp: 32.5
    Options for that are requiring allocation delay to be less than deallocation delay OR potentially only pushing to the modification queue for deallocations

@ypatil12 ypatil12 force-pushed the yash/regression-test-conflicting-times branch from 4c6141e to 4fc6339 Compare October 15, 2024 23:27
@ypatil12 ypatil12 force-pushed the yash/regression-test-conflicting-times branch from 4fc6339 to c8b74b4 Compare October 16, 2024 05:15
@ypatil12
Copy link
Collaborator Author

Closed in favor of #827

@ypatil12 ypatil12 closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant