Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature: move partition #1326
Feature: move partition #1326
Changes from 35 commits
924722e
da95dbb
e502709
eaf5370
2fa9693
8725420
d534373
36caece
badb691
fd39aac
daab781
0271369
c046cc6
3eac91e
181dc2e
8eb2243
394ea44
d1fdb0f
0804565
0353a28
bc9e6a1
c1d88a2
37d7d03
89bcb74
79ab15d
39d6447
0bd0611
c01b54c
ccdb45f
b775fa8
a5290cc
bf13558
9195aa1
70fd5fe
511e170
d16077c
9ed53b8
15d5ac1
5744223
e8f0a22
194b7cd
5e400d2
c1159c5
cb5e3c4
faa4873
7bc1156
f7b7bd7
3b25fdc
f3ddb23
9c8517a
3c6a374
271e3c0
d2202ab
d276241
b2dc66c
fcc4cc0
7619065
6e91261
9964257
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find these names a bit confusing, especially when they're elaborated below.
I think these correspond to
OrigDeadline
andDestDeadline
in the FIP below - consider sharing the language?https://github.com/filecoin-project/FIPs/blob/7b38d298f6f9d3f6bdf307a0074155c5acc8b425/FIPS/fip-0070.md#method-signature-and-parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two counts must be the same, because no sectors are faulty. So skip the bitfield difference caluclation of
live_sectors
and just use a singlesector_count
variable equal tosectors.len()
(and leave a comment explaining this).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of the code outside the block that is updating Partition could be factored out into methods on Deadline. We can then test these much more easily than by orchestrating actual moves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we don't want to update
Deadline.expirations_epochs
for eachPartition
, instead we want to do it in batch for all partitions being moved. It's semantically weird if this function updates all other fields exceptexpirations_epochs
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterating the deadline's whole expiration queue is fairly expensive. I don't think we need to.
This will iterate only the moved partition's queue, not any whole deadline's queue. It will be easier to fit this inside
Deadline::add/remove_partition
since you only need to access one deadline at a time. Again easier testing.Note that this approach is only ok on the precondition that there are no faulty sectors, which would appear twice in the source deadline and must be propagated at both epochs. Document this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's essentially as expensive as
expiration_epochs.cut
which is called inremove_partitions
, do we really want to save some gas by leaving some garbage data?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: why have you chosen raw comments rather than doc-comments for all the functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I was following the original style of
compact_partitions
:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look correct, but please add tests to demonstrate all the cases.