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.

Sign up for GitHub

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

Jump to bottom

chore: [MR-615] Refactor list_state_heights and make it an associated method #1690

Merged
merged 6 commits into from
Sep 30, 2024
Merged

chore: [MR-615] Refactor list_state_heights and make it an associated method #1690

merged 6 commits into from
Sep 30, 2024

Conversation

ShuoWangNSL
Copy link
Contributor

@ShuoWangNSL ShuoWangNSL commented Sep 26, 2024 •
edited
Loading

Close MR-615

This PR (1) excludes checkpoint_heights from the result of list_state_heights and (2) removes it from the StateReader trait and make it an associated method.

@ShuoWangNSL ShuoWangNSL force-pushed the shuo/list_only_inmem_state_heights branch from 8ae1ed3 to 54ac1fb Compare September 27, 2024 06:06
@ShuoWangNSL ShuoWangNSL changed the title Shuo/list only inmem state heights chore: [MR-615] Refactor list_state_heights and make it an associated method Sep 27, 2024
@github-actions github-actions bot added the chore label Sep 27, 2024
@ShuoWangNSL ShuoWangNSL force-pushed the shuo/list_only_inmem_state_heights branch from bd57acc to ea8f165 Compare September 27, 2024 06:48
@ShuoWangNSL
Copy link
Contributor Author

ShuoWangNSL commented Sep 27, 2024 •
edited
Loading

  1. Exclude checkpoint_heights from the result of list_state_heights

This MR is motivated by #906 where we want the method to only reflect states in memory.
Currently snapshots are kept at checkpoint heights so this PR does not have no immediate impact on the result of this method and existing tests should still pass.
However, #906 will introduce changes where snapshots could be removed at checkpoint heights. Thus, we don't want to mix checkpoint heights and snapshots heights in the result when making assertions in tests on what snapshots should exist.

  1. Remove it from the StateReader trait and make it an associated method.

As @derlerd-dfinity suggests, we would like to go one further step for the refactoring because list_state_heights is not used by any components in the current code base.
You may refer to this PR #1942 for the context where we introduced it as a method for trait StateReader.
Previous reasons for adding this trait method do not hold anymore as the design and code evolves. Within StateManager, it is merely used in tests and debug_assertions.
We can make it an associated method of StateManagerImpl.

@ShuoWangNSL ShuoWangNSL force-pushed the shuo/list_only_inmem_state_heights branch from cbce583 to f81efc0 Compare September 27, 2024 07:59
@ShuoWangNSL ShuoWangNSL marked this pull request as ready for review September 27, 2024 08:05
@ShuoWangNSL ShuoWangNSL requested review from a team as code owners September 27, 2024 08:05
@ShuoWangNSL ShuoWangNSL added this pull request to the merge queue Sep 30, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 30, 2024
@ShuoWangNSL
chore: [MR-615] Refactor list_state_heights and make it an associated…
c4ac15e 
… method (#1690)

Close [MR-615](https://dfinity.atlassian.net/browse/MR-615)

This PR (1) excludes checkpoint_heights from the result of
list_state_heights and (2) removes it from the StateReader trait and
make it an associated method.

[MR-615]:
https://dfinity.atlassian.net/browse/MR-615?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 30, 2024
@ShuoWangNSL ShuoWangNSL added this pull request to the merge queue Sep 30, 2024
Merged via the queue into master with commit c39a8b3 Sep 30, 2024
28 checks passed
@ShuoWangNSL ShuoWangNSL deleted the shuo/list_only_inmem_state_heights branch September 30, 2024 18:06
levifeldman pushed a commit to levifeldman/ic that referenced this pull request Oct 1, 2024
@ShuoWangNSL @levifeldman
chore: [MR-615] Refactor list_state_heights and make it an associated…
9e3da49 
… method (dfinity#1690)

Close [MR-615](https://dfinity.atlassian.net/browse/MR-615)

This PR (1) excludes checkpoint_heights from the result of
list_state_heights and (2) removes it from the StateReader trait and
make it an associated method.

[MR-615]:
https://dfinity.atlassian.net/browse/MR-615?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derlerd-dfinity derlerd-dfinity derlerd-dfinity approved these changes

@schneiderstefan schneiderstefan schneiderstefan approved these changes

Assignees
No one assigned
Labels
chore @ic-interface-owners @ic-message-routing-owners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShuoWangNSL @schneiderstefan @derlerd-dfinity