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

use separate batch for snapshot orphans #268

Closed
wants to merge 10 commits into from
Closed

Conversation

erikgrinaker
Copy link
Contributor

Fixes #261. I tried several approaches, this is the simplest one that passes all current tests.

I found it hard to reason about the interactions between orphaning and pruning, and am concerned that this only adds to the complexity, and thus the potential for harmful interactions. I will write a comprehensive, randomized stress test in a separate PR.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK, excited for the randomized tests.

nodedb.go Show resolved Hide resolved
nodedb.go Show resolved Hide resolved
nodedb.go Show resolved Hide resolved
nodedb.go Show resolved Hide resolved
@erikgrinaker
Copy link
Contributor Author

I realized this has at least two bugs:

  • Deleting a previously persisted version will flush queued orphans for non-persisted changes.

  • Deleting a previously persisted version will not take into account and adjust orphans queued since the last persisted version.

I'll come up with a fix for the former, but I don't believe the latter can be easily fixed without a pruning redesign.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 18, 2020

Allright, I think this should be good to review again - it's not pretty, but it works. I haven't fixed the second bug above, which requires a pruning redesign, but it's not likely to trigger for most use-cases (e.g. the SDK's usage pattern) - the consequence should only be stale orphans being left behind, I think.

The test failure is because of #269.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 18, 2020

The randomized test suite being built on erik/testageddon is showing some problems with this PR, it's unclear if it's caused by this change or if it's an unrelated problem since the suite panics on master. Will convert this to draft until investigated.

@erikgrinaker erikgrinaker marked this pull request as draft June 18, 2020 14:27
@erikgrinaker
Copy link
Contributor Author

No longer relevant, pruning was removed in #274.

@erikgrinaker erikgrinaker deleted the erik/issue-261 branch June 24, 2020 12:32
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.

IAVL panic with missing value
4 participants