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.
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
Peer backup #8490
base: master
Are you sure you want to change the base?
Peer backup #8490
Changes from all commits
52d310c
6d06ced
be3f077
637d6ab
4f70129
3322038
71027b8
5a8be6b
abbb190
8b0bd5b
ccecf4b
63c5f2d
562d40a
46d3a2a
b9d38e5
0b66b3d
9dedf1a
12d4c95
93af069
59f829b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Also unset the
ProvideStorageRequired
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.
I did not add that in the default config yet. Just thought we should only add the optional flag for now.
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.
Look at all the code immediately above here though... when the config variable that disables PeerStorage is set, it should Unset both feature bits. Unset is an idempotent operation and so can be performed even when the bit is not already set.
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.
Unsetting the required bit is something that would eventually done when it is part of the default config though but I can still added it here since it makes no difference
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.
unaddressed 🙏
Important to note here that we want code to be maintainable in the long term. If we ever change that default to be Required - we dont want to have to remember to come and change things here. A user setting the
NoPeerStorage
option, should always work.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.
I initially did not include it because I do not think maybe it makes sense to unset what was not initially set. If in the future the required bit is added to the default set. Then I think it would make sense to add the unsetting code here as well. Flows naturally to me and does not make this code unmaintainable in any way IMO.