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 how we check for the unsealed status of a piece in Shard migration #575

Merged

Conversation

aarshkshah1992
Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 commented Jul 16, 2021

@dirkmc

  1. We can have multiple deals for a Piece. We wanna know if there exists a single deal for the Piece that can be served from an Unsealed sector file.

  2. We can't pass offset=0,size=1 to check the unsealing status because the sector storage subsystem needs that infromation to determine if we an unsealed copy of the deal (even if a sector file is already unsealed, it's possible for it to have holes i.e. it only has some unsealed deals and dosen't have any data for others).

Comment on lines 116 to 118
if len(pinfo.Deals) <= 0 {
return xerrors.Errorf("no storage deals found for Piece %s", pcid)
}
Copy link
Member

Choose a reason for hiding this comment

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

length can never be below zero.

Suggested change
if len(pinfo.Deals) <= 0 {
return xerrors.Errorf("no storage deals found for Piece %s", pcid)
}
if len(pinfo.Deals) == 0 {
return xerrors.Errorf("no storage deals found for Piece %s", pcid)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -105,6 +105,8 @@ type StorageProviderNode interface {

// GetProofType gets the current seal proof type for the given miner.
GetProofType(ctx context.Context, addr address.Address, tok shared.TipSetToken) (abi.RegisteredSealProof, error)

IsUnsealed(ctx context.Context, sectorID abi.SectorNumber, offset abi.UnpaddedPieceSize, length abi.UnpaddedPieceSize) (bool, error)
Copy link
Member

Choose a reason for hiding this comment

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

nit: godoc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

n.lk.Lock()
defer n.lk.Unlock()

sealed := n.Sealed[sectorID]
Copy link
Member

Choose a reason for hiding this comment

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

If a sector is not tracked, this will return the zero value of the bool type which is false, and subsequently we'd return true, nil. We should fail if the map existence check fails.

(I know this is a stub for testing only, but let's fix it so that it doesn't send us down time-consuming debugging trips in the future!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment on lines -26 to -30
type SectorStateAccessor interface {
StateSectorGetInfo(context.Context, address.Address, abi.SectorNumber, types.TipSetKey) (*miner.SectorOnChainInfo, error)
IsUnsealed(ctx context.Context, sector storage.SectorRef, offset storiface.UnpaddedByteIndex, size abi.UnpaddedPieceSize) (bool, error)
}

Copy link
Member

Choose a reason for hiding this comment

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

nit: I actually preferred @dirkmc's method of tightening the interface requirement to only the 2 methods that are needed. It's more idiomatic and specific (defining interface at the point of use). This new approach leaks, in sense that now the ShardMigrator is now privy to the fact that these two behaviours are provided by two different system components.

That said, no need to revert this at all, it's absolutely not a big deal here because this component has a limited lifetime and is not reusable.

@dirkmc
Copy link
Contributor

dirkmc commented Jul 19, 2021

even if a sector file is already unsealed, it's possible for it to have holes

Nice catch, I didn't realize this 👍

@aarshkshah1992 aarshkshah1992 merged commit de11811 into feat/wip-markets-dagstore Jul 19, 2021
@aarshkshah1992 aarshkshah1992 deleted the fix/shard-migration-isunsealed branch July 19, 2021 08:22
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.

3 participants