-
Notifications
You must be signed in to change notification settings - Fork 407
Allow keys to be unpersisted #1705
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
Closed
tnull
wants to merge
3
commits into
lightningdevkit:main
from
tnull:2022-09-kvstorepersister-unpersist
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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.
Could you add motivation for adding this and where it will or can be used?
Depending on use-case, Ideally i would like it to be just former, to support some of the systems which support async delete. (And in this case we might not care much if it doesn't succeed immediately or is not strongly consistent.)
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.
Do you mean add to motivation in comments, in the commit message, or here on GitHub?
I generally think that it would be nice to have
list
andunpersist
functionalities, if the persister is going to be used for a wider array of objects. Moreover, the sample node andLdkLite
currently just bypass theFilesystemPersister
for initalization (i.e., they read the directory contents viastd::fs
methods), which is no option if we'd ever want to have another persister backend.However, my concrete motivation was the initial draft of a
PaymentInfoStore
forLdkLite
(lightningdevkit/ldk-node#13) which uses the persister, where I however didn't want to store one huge blob of data under a single key. I therefore now store each payment info separately using the payment hash as a key. It would however be nice to be able to prune that store eventually, hence the need for anunpersist
method.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 meant in PR description.(for future reference as well)
Got it, from your description our intention is space optimization. (i.e. pruning)
It would be best if we add to documentation of this api that this will be used for pruning. and hence we can utilize async deletes or eventually consistent deletes.
This also means:
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 tend to disagree. Yes, the current motivation for this is pruning some data which is fine to be delayed, which however might not be the case for other future use cases. It would also pretty annoying to just silently fail to unpersist for some reason. But, more importantly, I think it would be very confusing to have an interface in which different but very similarly named methods behave differently or provide very different consistency guarantees. I'd therefore argue that while data removal is almost always not as critical as persistence, we should try to align the guarantees two methods as far as possible?
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.
Another way is to have a flag, which allows for both. (if you think there will be future requirement for it and feel strongly about it) (imo we should keep it for pruning purpose to start with or by Default)
Not all systems provide same guarantees while deleting and we should think beyond filesystem-persistence.
And in LN and LDK usecase, persist and delete can live with different consistency requirements. In all case we need persist to be strongly consistent, whereas its not the same for delete.
The concern for "methods behave differently" is valid but that doesn't mean we should make api or consistency decisions based on that.
In which case do we expect immediate delete?
We already have big overhaul ongoing for async persist, and if we can be conservative while adding delete functionality then why not ?
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 don't think a separate key per payment is the right approach here, either? That is a lot of keys, and with very small values. Generally many filesystems are really bad at folders with lots of keys, as many operations expect to load the full set of keys in a folder at a time, so I don't think we can do that in general.
I agree that the motivation here needs to be scoped a bit better. Concretely, we want to move towards removing ChannelMonitors in the future, but I'm not actually sure that we really ever want to fully remove a monitor - we just want to "archive" them. I worry if we have a bug we may need a monitor to recover and claim some funds in the future, so would prefer we just archive and not delete.
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.
Yes, but I think the current
PaymentInfoStore
draft should really be a starting point, since we want a database backend for these kinds of data anyways. So regarding that concrete use case I'd argue we still want to addunpersist
to the trait, but implement aKVStorePersister
with a different backend in LDKLite (probably using thesled
database at least initially, since it's already in use for BDK persistence).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.
Right, but then presumably the
unpersist
function doesn't belong in the upstream trait but rather in your local trait for database logic, as its not reasonable to expect all implementors to be used for that functionality?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.
Hm, but I don't see why we then have an upstream trait at all, if it doesn't capture functionalities that ~all implementations would need? Even more so than for
unpersist
this is true forlist
andget
functionalities, which literally every user/implementation will need. Take for exampleldk-sample
which of course also needs these functionalities for initialization, but solves the problem by simply bypassing the trait and evenFilesystemPersister
by just going throughstd::fs
and reading the data stored at the specific hardcoded locations. However, this of course doesn't work for all backend implementations, so while the trait is supposed to enable different backends, it currently just works if we assume storing data in files.In particular if we want to make the persister configurable for the user, e.g., in LDKLite, the
KVStorePersister
trait needs to offer a way to list and retrieve the data stored for a specific key. Of course this could be solved by yet another augmenting downstream trait specific to LDKLite, but this is not the idea of having a generalKVStorePersister
trait?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.
The upstream trait should capture the things that upstream needs. If users want to add methods, they should totally do that, but its not of upstream's concern?
Yes, I think we should list and get to the upstream trait, and then use that to implement initialization methods upstream, as utilities for the user. Thus using those methods :)