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

feat: add seal resume testing to window post bench #1288

Merged
merged 3 commits into from
Sep 25, 2020

Conversation

cryptonemo
Copy link
Collaborator

No description provided.

@cryptonemo
Copy link
Collaborator Author

@dignifiedquire This PR adds the testing to benchy's window post and fixes the CI tests for your PR (should be good if merged). The last thing I think we need to ensure resume correctness is to add code that deletes all layers after the layer that is starting after resume (I think this can be added to your clear_temp logic). An alternative is to assume that any/all future ones are ok, or somehow verify them, but given the feature as a first cut, I think the safest option is to delete future ones.

The case I'm thinking of is a replication with 11 layers in progress, and we complete 10 layers. For whatever reason, layer 7 is deleted before resume, so we resume at building layer 7. We could assume or verify later layers (i.e. 8-10) are still good, or just delete all later ones and re-generate them. Since they're not deleted now, it's possible the code will fail in this case.

If you have thoughts on this, let me know. Also, if you want me to add this fix to this PR or yours (if this is merged), let me know.

@dignifiedquire
Copy link
Contributor

For now I have assumed that any layers that exist are valid, even if there are "holes" in total. Deleting/overwriting the following layers might be a safer version I agree. I don't feel strongly in any direction though.

@dignifiedquire dignifiedquire merged commit cd0c828 into feat/seal-resume Sep 25, 2020
@dignifiedquire dignifiedquire deleted the seal-resume-test branch September 25, 2020 12:10
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.

2 participants