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

integrate extern/{storage-fsm,sector-storage} into lotus source tree #3089

Merged
merged 8 commits into from
Aug 17, 2020

Conversation

raulk
Copy link
Member

@raulk raulk commented Aug 16, 2020

  • Move extern/storage-fsm into storage/sealing package; drop replace directive.
  • Move extern/sector-storage into storage/sector package; drop replace directive.
  • Drop unneeded import aliases, because package names are consistent now.
  • rm README, CircleCI config, LICENSES, etc. and other irrelevant artifacts from migrated repos.
  • Introduce a go generate directive for CBOR generation in storage/sealing. Make Makefile call it.
  • Fix lint issues in sector-storage.

@raulk raulk requested review from Kubuxu, magik6k and arajasek August 16, 2020 09:51
@raulk raulk changed the title integrate extern/storage-fsm into lotus source tree integrate extern/{storage-fsm,sector-storage} into lotus source tree Aug 16, 2020
@raulk raulk force-pushed the integrate/storage-fsm branch from 0e1b59f to 2b511d2 Compare August 16, 2020 10:44
@vyzo
Copy link
Contributor

vyzo commented Aug 16, 2020

test failure seems real and not a fluke; some file is missing:

--- FAIL: TestDownloadParams (0.00s)
panic: open ../parameters.json: no such file or directory [recovered]
	panic: open ../parameters.json: no such file or directory

goroutine 15 [running]:
testing.tRunner.func1.1(0x245ca40, 0xc000290630)
	/usr/local/go/src/testing/testing.go:988 +0x30d
testing.tRunner.func1(0xc0001b37a0)
	/usr/local/go/src/testing/testing.go:991 +0x3f9
panic(0x245ca40, 0xc000290630)
	/usr/local/go/src/runtime/panic.go:969 +0x166
github.com/filecoin-project/lotus/storage/sector/ffiwrapper.getGrothParamFileAndVerifyingKeys(0x800)
	/home/circleci/project/storage/sector/ffiwrapper/sealer_test.go:227 +0x113
github.com/filecoin-project/lotus/storage/sector/ffiwrapper.TestDownloadParams(0xc0001b37a0)
	/home/circleci/project/storage/sector/ffiwrapper/sealer_test.go:245 +0x71

@raulk
Copy link
Member Author

raulk commented Aug 16, 2020

@vyzo hopefully this will fix it: 4f6d01d

@raulk
Copy link
Member Author

raulk commented Aug 16, 2020

@vyzo I suspect that if the test is downloading proof parameters, it has the potential to be a time sink. We may need to create a different CircleCI job for this.

@vyzo
Copy link
Contributor

vyzo commented Aug 16, 2020

yeah, by no means ideal.

@raulk
Copy link
Member Author

raulk commented Aug 16, 2020

@vyzo all passing here; looks like the last failures were flaky tests.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

  • sector isn't a good name for a subsystem which is handling storage and is scheduling sealing operations
    • There are probably better names, but I'd keep sectorstorage for now, also minimizes the diff size
  • I'd keep those packages in extern/, the intention is to extract them back out of lotus when GFC development resumes

@raulk raulk closed this Aug 17, 2020
@raulk raulk reopened this Aug 17, 2020
@raulk raulk requested a review from magik6k August 17, 2020 13:31
@raulk raulk force-pushed the integrate/storage-fsm branch from 5aacc83 to 862bafc Compare August 17, 2020 13:39
@raulk
Copy link
Member Author

raulk commented Aug 17, 2020

@magik6k here's a version that keeps storage-fsm (now renamed to storage-sealing) and sector-storage under extern. I also reverted the sectorstorage=>sector package rename.

With this, your review comment should be addressed.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Just 1 nit

int64(stat.Blocks) * 512, // NOTE: int64 cast is needed on osx
stat.Blocks * 512, // NOTE: int64 cast is needed on osx
Copy link
Contributor

Choose a reason for hiding this comment

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

This cast needs to be here

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this! Will revert and add a // nolint here.

@raulk raulk force-pushed the integrate/storage-fsm branch from 68a548b to 1f01626 Compare August 17, 2020 16:26
@raulk raulk requested a review from magik6k August 17, 2020 16:31
@raulk
Copy link
Member Author

raulk commented Aug 17, 2020

@magik6k fixed.

@magik6k magik6k merged commit 6ef7a30 into next Aug 17, 2020
@magik6k magik6k deleted the integrate/storage-fsm branch August 17, 2020 16:37
@fadeAce
Copy link

fadeAce commented Aug 20, 2020

  • sector isn't a good name for a subsystem which is handling storage and is scheduling sealing operations

    • There are probably better names, but I'd keep sectorstorage for now, also minimizes the diff size
  • I'd keep those packages in extern/, the intention is to extract them back out of lotus when GFC development resumes

it's better to be called manager cause it's a scheduler and sector manangement executor

@fadeAce
Copy link

fadeAce commented Aug 20, 2020

  • Move extern/storage-fsm into storage/sealing package; drop replace directive.
  • Move extern/sector-storage into storage/sector package; drop replace directive.
  • Drop unneeded import aliases, because package names are consistent now.
  • rm README, CircleCI config, LICENSES, etc. and other irrelevant artifacts from migrated repos.
  • Introduce a go generate directive for CBOR generation in storage/sealing. Make Makefile call it.
  • Fix lint issues in sector-storage.

what I do suspect is that will this change to happen in master & mainnet release ? Moving a mod to subtree if it's needed to be done why not drop storage-fsm project in repo

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.

4 participants