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

test(storage provider pallet): add setup for multiple partitions #194

Merged
merged 17 commits into from
Aug 14, 2024
Merged

test(storage provider pallet): add setup for multiple partitions #194

merged 17 commits into from
Aug 14, 2024

Conversation

cernicc
Copy link
Member

@cernicc cernicc commented Aug 13, 2024 •
edited
Loading

Description

This PR contains multiple changes:

  • Implement test for marking sectors as faulty in multiple partitions
  • In deadline, start adding new sectors to the last partition because we know that the previous are already full
  • The declare_faults failed if we had multiple partitions and didn't mention all partitions in the fault declaration.
  • Check and return an error if we try to add empty sectors to the SectorMap. I am not sure this was the best location to handle the case when fault declaration contained an empty set.
  • This pr also tests for SP: declare_faults wrong specified faults #190 and partially fixes it.

Checklist

@cernicc cernicc self-assigned this Aug 13, 2024
@cernicc cernicc linked an issue Aug 13, 2024 that may be closed by this pull request
SP Pallet (test): Add setup for multiple partitions #183
Closed
@cernicc cernicc added the ready for review Review is needed label Aug 13, 2024
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Aug 13, 2024
@jmg-duarte jmg-duarte added this to the Phase 1 milestone Aug 13, 2024
@jmg-duarte jmg-duarte added the pallet-storage-provider Relates to the Storage Provider Pallet label Aug 13, 2024
th7nder
th7nder previously requested changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

blocked on #180

@cernicc cernicc marked this pull request as draft August 14, 2024 06:28
@cernicc cernicc marked this pull request as ready for review August 14, 2024 09:10
@cernicc cernicc requested a review from th7nder August 14, 2024 09:10
neutrinoks
neutrinoks reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@neutrinoks neutrinoks left a comment

Choose a reason for hiding this comment

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

LGTM 👍 but I am still no pro in this topic 😆

jmg-duarte
jmg-duarte requested changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

Great job on the comments!

Some changes and questions

pallets/storage-provider/src/deadline.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/sector_map.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/sector_map.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/sector_map.rs Show resolved Hide resolved
pallets/storage-provider/src/tests/declare_faults.rs Show resolved Hide resolved
pallets/storage-provider/src/tests/declare_faults.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/deadline.rs Show resolved Hide resolved
pallets/storage-provider/src/deadline.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/deadline.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/tests/declare_faults.rs Outdated Show resolved Hide resolved
aidan46
aidan46 previously approved these changes Aug 14, 2024
View reviewed changes
@aidan46 aidan46 self-requested a review August 14, 2024 11:44
@cernicc cernicc mentioned this pull request Aug 14, 2024
Open
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Aug 14, 2024
jmg-duarte
jmg-duarte approved these changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

LGTM

pallets/storage-provider/src/tests/declare_faults_recovered.rs Show resolved Hide resolved
@jmg-duarte jmg-duarte dismissed th7nder’s stale review August 14, 2024 13:43

Blocker has been merged

@jmg-duarte jmg-duarte merged commit f126c18 into develop Aug 14, 2024
3 checks passed
@jmg-duarte jmg-duarte deleted the test/183/test-add-setup-for-multiple-partitions branch August 14, 2024 13:43
@th7nder th7nder mentioned this pull request Aug 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aidan46 aidan46 aidan46 approved these changes

@jmg-duarte jmg-duarte jmg-duarte approved these changes

@th7nder th7nder Awaiting requested review from th7nder

@neutrinoks neutrinoks Awaiting requested review from neutrinoks

Assignees

@cernicc cernicc

Labels
pallet-storage-provider Relates to the Storage Provider Pallet ready for review Review is needed
Projects
None yet
Milestone
Phase 1
Development

Successfully merging this pull request may close these issues.

SP Pallet (test): Add setup for multiple partitions
5 participants
@cernicc @th7nder @jmg-duarte @aidan46 @neutrinoks