Bypass task scheduler for reading unsealed pieces #6280

aarshkshah1992 · 2021-05-18T07:33:29Z

Picks the minimal set of changes we need from @magik6k's PR at #6128.

TODO

dirkmc

Looking pretty good 👍 - Just a few nits

extern/sector-storage/manager.go

extern/sector-storage/piece_provider.go

extern/sector-storage/stores/http_handler.go

extern/sector-storage/stores/remote.go

dirkmc · 2021-05-18T22:20:59Z

I suggest in the description of the PR we add a checkbox list of things that have been done (implement change) and things that need to be done before this PR is ready to be reviewed (adding tests)

aarshkshah1992 · 2021-05-19T05:55:45Z

extern/sector-storage/stores/remote.go

+			return nil, xerrors.Errorf("failed to read sector %v from remote(%d): %w", s, ft, storiface.ErrSectorNotFound)
+		}
+
+		// TODO Why are we sorting in ascending order here -> shouldn't we sort in descending order as higher weight means more likely to have the file ?


@magik6k

This looks incorrect, right ? We should sort in descending order of weight because a higher weight means a higher probability that it'll have the unsealed file we are looking for here.

aarshkshah1992

@dirkmc Have addresses all your review comments. Unit Tests are WIP.

extern/sector-storage/stores/remote.go

extern/sector-storage/piece_provider.go

magik6k · 2021-05-19T10:19:50Z

extern/sector-storage/stores/remote.go

+		return nil, xerrors.Errorf("failed to read sector %v from remote(%d): %w", s, ft, storiface.ErrSectorNotFound)
+	}
+
+	// TODO Why are we sorting in ascending order here -> shouldn't we sort in descending order as higher weight means more likely to have the file ?


Yeah, that's probably a bug, also in acquireFromRemote

It shouldn't have much impact, as in most cases sectors are only in one storage, which is likely why it wasn't noticed

magik6k

First review pass, looks good.

We should also bump the minor miner/worker api versions in api/version.go given that the storage http api now has the new endpoint

Also ideally we'd test this on a mainnet miner with some workers before merging just to confirm it doesn't have issues in real-world setups

dirkmc

Looks good Aarsh 👍

I'll hold off on approving until we have the tests in place

dirkmc · 2021-05-19T11:15:14Z

extern/sector-storage/storiface/ffi.go

@@ -17,6 +18,14 @@ func (i UnpaddedByteIndex) Padded() PaddedByteIndex {
 	return PaddedByteIndex(abi.UnpaddedPieceSize(i).Padded())
 }

+func (i UnpaddedByteIndex) Valid() error {
+	if i%127 != 0 {
+		return xerrors.Errorf("unpadded byte index must be a multiple of 127")


Suggested change

return xerrors.Errorf("unpadded byte index must be a multiple of 127")

return xerrors.Errorf("unpadded byte index is %d but must be a multiple of 127", i)

aarshkshah1992 · 2021-05-19T13:54:41Z

@magik6k

We are working on some solids unit tests & integration tests for this PR and you should see them here in some days.
Once the unit & integration tests are in place, we'll test it on a devnet with 8K Sectors and on the mainnet too to ensure things are good.

extern/sector-storage/piece_provider_test.go

magik6k · 2021-05-21T09:37:09Z

extern/sector-storage/storiface/worker.go

@@ -87,7 +86,6 @@ type WorkerCalls interface {
 	ReleaseUnsealed(ctx context.Context, sector storage.SectorRef, safeToFree []storage.Range) (CallID, error)
 	MoveStorage(ctx context.Context, sector storage.SectorRef, types SectorFileType) (CallID, error)
 	UnsealPiece(context.Context, storage.SectorRef, UnpaddedByteIndex, abi.UnpaddedPieceSize, abi.SealRandomness, cid.Cid) (CallID, error)
-	ReadPiece(context.Context, io.Writer, storage.SectorRef, UnpaddedByteIndex, abi.UnpaddedPieceSize) (CallID, error)


extern/sector-storage/stores/util_unix.go

magik6k · 2021-05-21T09:54:23Z

extern/sector-storage/piece_provider_test.go

+	svc := &http.Server{
+		Addr:    nl.Addr().String(),
+		Handler: mux,
+	}


Could use https://golang.org/pkg/net/http/httptest if it would simplify things

extern/sector-storage/stores/http_handler.go

extern/sector-storage/stores/remote.go

nonsense · 2021-05-21T10:43:39Z

extern/sector-storage/piece_provider.go

+
+	var uns bool
+
+	if r == nil {


if r == nil we don't have to call unlock ? Because we do have a few returns in this branch that are not calling unlock...?

If r == nil unlock is called within tryReadUnsealedPiece:
https://github.com/filecoin-project/lotus/pull/6280/files/40642b2cad0bb247dd789837c6465163e5b59742#diff-1fe5f948c5ef6d7d11919b998fedd74927b0cfac11ebb3ef1dbaf518fc026999R61

I found this very hard to follow as well, I wonder if there's a way we can refactor to simplify

dirkmc

Looking really good Aarsh, I love these comprehensive unit and integration tests 👍 👍

extern/sector-storage/stores/remote.go

extern/sector-storage/piece_provider.go

aarshkshah1992 · 2021-05-21T13:47:37Z

@magik6k @dirkmc @nonsense Have addressed your review. Please take a look.

magik6k

Looks great, if testing on mainnet goes well we can probably just merge

aarshkshah1992 · 2021-06-07T13:15:01Z

@magik6k The failing tests look like flakies. Please can you merge the PR if you are happy ?

magik6k · 2021-06-07T13:44:48Z

build-lotus-soup is failing (and not flaky)

aarshkshah1992 · 2021-06-07T14:07:06Z

@magik6k The build-lotus-soup job has been fixed. Please let's ship this ! :) 🚢

aarshkshah1992 requested review from arajasek and magik6k as code owners May 18, 2021 07:33

aarshkshah1992 marked this pull request as draft May 18, 2021 07:33

aarshkshah1992 requested review from dirkmc and nonsense May 18, 2021 12:33

aarshkshah1992 assigned raulk May 18, 2021

dirkmc reviewed May 18, 2021

View reviewed changes

aarshkshah1992 commented May 19, 2021

View reviewed changes

extern/sector-storage/stores/remote.go Outdated Show resolved Hide resolved

extern/sector-storage/piece_provider.go Show resolved Hide resolved

extern/sector-storage/piece_provider.go Show resolved Hide resolved

extern/sector-storage/piece_provider.go Show resolved Hide resolved

magik6k reviewed May 19, 2021

View reviewed changes

dirkmc reviewed May 19, 2021

View reviewed changes

dirkmc force-pushed the feat/pieceread-outside-scheduler branch from 9c6462f to ed4d54b Compare May 19, 2021 20:39

aarshkshah1992 commented May 20, 2021

View reviewed changes

extern/sector-storage/piece_provider_test.go Outdated Show resolved Hide resolved

dirkmc force-pushed the feat/pieceread-outside-scheduler branch 2 times, most recently from b606f8a to c17300d Compare May 20, 2021 21:25

magik6k reviewed May 21, 2021

View reviewed changes

jacobheun added the team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs label May 21, 2021

nonsense reviewed May 21, 2021

View reviewed changes

dirkmc reviewed May 21, 2021

View reviewed changes

extern/sector-storage/stores/remote.go Show resolved Hide resolved

extern/sector-storage/piece_provider.go Show resolved Hide resolved

dirkmc approved these changes May 21, 2021

View reviewed changes

magik6k approved these changes May 21, 2021

View reviewed changes

magik6k mentioned this pull request May 21, 2021

retrieval: Direct piecereader #6128

Closed

dirkmc mentioned this pull request May 21, 2021

Read Pieces by bypassing the scheduler and add extra logging for retrievals #6313

Closed

magik6k mentioned this pull request May 31, 2021

Support standalone miner-market process #6356

Merged

19 tasks

aarshkshah1992 added 2 commits June 7, 2021 15:02

bypass task scheduler for reading unsealed pieces

670835f

docs, logs and green ci

65eb610

aarshkshah1992 and others added 16 commits June 7, 2021 15:02

test http handler

759d8f0

fix linting problems

74372d3

feat: TestPieceProviderReadPiece

6879ae9

use an actual worker in the integration tests

77b5e8d

use mockgen

0d88800

unit tests for the remote store Reader

4efaa8d

fix go mod

35a0dbf

remove read task type and run gen and docsgen

ad4b182

finish integration tests

acfa3d7

more logging

22f3648

better logging

3b792a3

clean up logging

73f7825

integration test should remove unsealed files

207f0d9

logs to debug read and unseal

ec6a496

logs to debug read & unseal

2a13488

changes as per review

8d9cef1

aarshkshah1992 marked this pull request as ready for review June 7, 2021 10:15

finished rebasing PR

21e6b50

aarshkshah1992 force-pushed the feat/pieceread-outside-scheduler branch from 50e023e to 21e6b50 Compare June 7, 2021 10:32

aarshkshah1992 added 2 commits June 7, 2021 16:12

update ffi

1a90d3b

fix CI

ddd9bf6

aarshkshah1992 changed the title ~~[WIP Draft] Bypass task scheduler for reading unsealed pieces~~ Bypass task scheduler for reading unsealed pieces Jun 7, 2021

fix lotus soup build

7124cd5

Bump miner/worker api versions

8625da3

magik6k approved these changes Jun 7, 2021

View reviewed changes

magik6k merged commit fadc79a into master Jun 7, 2021

magik6k deleted the feat/pieceread-outside-scheduler branch June 7, 2021 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bypass task scheduler for reading unsealed pieces #6280

Bypass task scheduler for reading unsealed pieces #6280

aarshkshah1992 commented May 18, 2021 •

edited

Loading

dirkmc left a comment

dirkmc commented May 18, 2021

aarshkshah1992 May 19, 2021

aarshkshah1992 left a comment

magik6k May 19, 2021

magik6k May 19, 2021

magik6k left a comment

dirkmc left a comment

dirkmc May 19, 2021

aarshkshah1992 commented May 19, 2021

magik6k May 21, 2021

magik6k May 21, 2021

nonsense May 21, 2021

dirkmc May 21, 2021

dirkmc left a comment

aarshkshah1992 commented May 21, 2021

magik6k left a comment

aarshkshah1992 commented Jun 7, 2021

magik6k commented Jun 7, 2021

aarshkshah1992 commented Jun 7, 2021

	return xerrors.Errorf("unpadded byte index must be a multiple of 127")
	return xerrors.Errorf("unpadded byte index is %d but must be a multiple of 127", i)

Bypass task scheduler for reading unsealed pieces #6280

Bypass task scheduler for reading unsealed pieces #6280

Conversation

aarshkshah1992 commented May 18, 2021 • edited Loading

TODO

dirkmc left a comment

Choose a reason for hiding this comment

dirkmc commented May 18, 2021

Choose a reason for hiding this comment

aarshkshah1992 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

dirkmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmc left a comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 21, 2021

magik6k left a comment

Choose a reason for hiding this comment

aarshkshah1992 commented Jun 7, 2021

magik6k commented Jun 7, 2021

aarshkshah1992 commented Jun 7, 2021

aarshkshah1992 commented May 18, 2021 •

edited

Loading