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

Refactor initializer to be simpler to use and race free #84

Merged
merged 12 commits into from
Nov 9, 2022

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Nov 7, 2022

This fixes #78 by slimming down the Initializer and removing asynchronous calls. This should also fix data race issues we have with the component.

I did not remove the Reset method, since it is used by go-spacemesh and factoring it out would have been more effort than what I think is worth at the moment.

Now Initializer::Initialize() takes a context that can be canceled by the caller to stop the initialization early. Initializer::Stop() was removed as it is not needed any more and instead of having multiple methods that report some kind of state (Started(), Completed(), etc.) there is now only one method called Status() that reports the state of the initialization.

For integration into go-spacemesh see: spacemeshos/go-spacemesh#3715

@fasmat fasmat self-assigned this Nov 7, 2022
@fasmat fasmat requested review from poszu, moshababo, dshulyak and countvonzero and removed request for poszu November 8, 2022 11:27
@fasmat fasmat marked this pull request as ready for review November 8, 2022 11:27
Copy link
Collaborator

@poszu poszu left a comment

Choose a reason for hiding this comment

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

Great work! I left few comments, some of them are just nits.

initialization/initialization.go Outdated Show resolved Hide resolved
initialization/initialization.go Show resolved Hide resolved
initialization/initialization.go Outdated Show resolved Hide resolved
initialization/initialization.go Show resolved Hide resolved
initialization/initialization_test.go Outdated Show resolved Hide resolved
initialization/initialization_test.go Outdated Show resolved Hide resolved
initialization/initialization_test.go Outdated Show resolved Hide resolved
initialization/initialization_test.go Show resolved Hide resolved
initialization/initialization_test.go Outdated Show resolved Hide resolved
proving/proving_test.go Outdated Show resolved Hide resolved
@fasmat fasmat requested a review from poszu November 8, 2022 23:48
@fasmat fasmat merged commit 87da468 into develop Nov 9, 2022
@fasmat fasmat deleted the 78-refactor-initializer branch November 9, 2022 15:38
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Nov 9, 2022
## Motivation
Integrates changes of spacemeshos/post#78 into go-spacemesh, only merge after the former has been merged.

## Changes
The asynchronous `Initializer::SessionNumLabelsWrittenChan()` is removed in favor of the synchronous `Initializer::SessionNumLabelsWritten()` with the changes in spacemeshos/post#84

This updates go-spacemesh to not use the removed method any more.

The GRPC server now sends PoST status updates to a client in 1 second intervals instead of relying on PoST to report its status in (possibly) irregular intervals. This also means that a client now has to cancel a request or it will receive status updates about PoST indefinitely.

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Nov 9, 2022
## Motivation
Integrates changes of spacemeshos/post#78 into go-spacemesh, only merge after the former has been merged.

## Changes
The asynchronous `Initializer::SessionNumLabelsWrittenChan()` is removed in favor of the synchronous `Initializer::SessionNumLabelsWritten()` with the changes in spacemeshos/post#84

This updates go-spacemesh to not use the removed method any more.

The GRPC server now sends PoST status updates to a client in 1 second intervals instead of relying on PoST to report its status in (possibly) irregular intervals. This also means that a client now has to cancel a request or it will receive status updates about PoST indefinitely.

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Nov 9, 2022
## Motivation
Integrates changes of spacemeshos/post#78 into go-spacemesh, only merge after the former has been merged.

## Changes
The asynchronous `Initializer::SessionNumLabelsWrittenChan()` is removed in favor of the synchronous `Initializer::SessionNumLabelsWritten()` with the changes in spacemeshos/post#84

This updates go-spacemesh to not use the removed method any more.

The GRPC server now sends PoST status updates to a client in 1 second intervals instead of relying on PoST to report its status in (possibly) irregular intervals. This also means that a client now has to cancel a request or it will receive status updates about PoST indefinitely.

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
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.

Refactor status updates by PoST Initializer
2 participants