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

Fix indexesValidated and PrefillEstimates to operate on absolute idx #454

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

udpatil
Copy link
Contributor

@udpatil udpatil commented Mar 8, 2024

Describe your changes and provide context

This is one component that was missed when refactoring to use absolute indices for EVM changes. This change refactors such that prefill estimates will appropriately fill the estimates by absolute Index and indexes validated will similarly check via absolute indices instead of relative.

Testing performed to validate your change

Existing unit tests + loadtesting

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 94.44444% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 55.35%. Comparing base (01cf97f) to head (1dae97e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            seiv2     #454   +/-   ##
=======================================
  Coverage   55.35%   55.35%           
=======================================
  Files         629      629           
  Lines       53817    53823    +6     
=======================================
+ Hits        29791    29796    +5     
- Misses      21896    21897    +1     
  Partials     2130     2130           
Files Coverage Δ
tasks/scheduler.go 93.91% <94.44%> (+0.10%) ⬆️

... and 1 file with indirect coverage changes

mappedWritesets := req.EstimatedWritesets
// order shouldnt matter for storeKeys because each storeKey partitioned MVS is independent
for storeKey, writeset := range mappedWritesets {
// we use `-1` to indicate a prefill incarnation
s.multiVersionStores[storeKey].SetEstimatedWriteset(i, -1, writeset)
s.multiVersionStores[storeKey].SetEstimatedWriteset(req.AbsoluteIndex, -1, writeset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be easy to add a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good point, we have a simple unit test to assert the proper values in the MultiVersionStore, I'll add one in

@udpatil udpatil merged commit e9376f4 into seiv2 Mar 11, 2024
13 checks passed
@udpatil udpatil deleted the fix-absolute-index-behavior branch March 11, 2024 16:26
udpatil added a commit that referenced this pull request Mar 11, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Mar 14, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Mar 26, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Mar 26, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Mar 27, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Apr 16, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Apr 19, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
udpatil added a commit that referenced this pull request Apr 19, 2024
…454)

This is one component that was missed when refactoring to use absolute
indices for EVM changes. This change refactors such that prefill
estimates will appropriately fill the estimates by absolute Index and
indexes validated will similarly check via absolute indices instead of
relative.

Existing unit tests + loadtesting
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.

5 participants