This repository has been archived by the owner on Feb 3, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lucksus
commented
Feb 27, 2019
EntryType::LinkRemove => retry_validation(boxed.clone(), context.clone()), | ||
_ => panic!("Pending validations are (currently) only implemented for links"), | ||
}); | ||
.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get this clone of a HashMap<Arc<...>>
so we can drop the state lock immediately.
lucksus
requested review from
maackle,
thedavidmeister and
zippy
and removed request for
maackle
February 27, 2019 19:41
zippy
approved these changes
Feb 27, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one minor change in test. and questions, but general approval
All addressed, @zippy. |
1 task
thedavidmeister
approved these changes
Feb 28, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Immediate follow-ups of the pending link validation merge
Builds on the work of #1054
Pending because of validation package
I couldn't easily write a test for pending link validations because having a node fake a link entry without actually validating it will also break the generation of validation packages.
That pointed me to the fact that validation can also fail early because we couldn't get the validation package from the source, usually because the source is offline.
I think we should store validation packages in the DHT to circumvent this long-term. For now, retrying validation of any entry in this case is the bare minimum we need to not break the DHT's consistency (nodes online during authoring can validate, future ones won't if source is offline).
Removing pending entries from pending list
We have to make sure that entries are removed from the pending list not only when validation succeeds, but also when it finally fails. Therefore added
HolochainError::ValidationPending
to tell pending cases apart from other failures. The scheduled background task now removes a processed item from the list unless it returnsHolochainError::ValidationPending
.Memory management
Entries to be validated are now
Arc
ed. The map of these Arcs gets copied out of the state so we can drop the state lock immediately to not block the instance while running pending validations.