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
feat: ADR 040: New DB interface #9573
feat: ADR 040: New DB interface #9573
Changes from all commits
05f1fce
3e9e56d
3d9dc49
80f9e6a
8b5d7cb
69e5a9a
fd9b9aa
dbdb1c9
5533111
d0f8927
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.
do we need this function?
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.
This will be needed for ABCI state sync, since snapshots need to be imported at a specific height.
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.
my inclination is that you'd specify iteration direction as an option to the method that produes the iterator and not require duplicate methods.
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.
Yeah, that's a trivial change. Any other opinions on this?
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 have a strong preference, but tend to have the opposite inclination 🤷♂️
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.
keeping the interface minimal is a desired design. However it shouldn't on a cost of overloading the functions.
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 also don't have a strong opinion here, but we use different methods in the SDK currently. Being as we're only adding one parameter, I don't see 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've written/used iterators with the pattern:
Which collapses Valid/Next,
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.
Seems fine, the only odd thing about this is it requires the iterator to start in an invalid state
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.
👍 - had similar opinion about it above.
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.
Thinking a bit more on this, it may not be ideal as it violates the single-responsibility principle. The interface is smaller but you can't check the iterator's state without mutating it.
That said, I have pushed the change to the interface.
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.
These methods should probably return an error, for forward compatibility with other potential implementations. E.g. if we change All() to using an iterator, the iterator may hit errors.
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.
This is true for all the interfaces, not just this one.
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'm going to push back a bit on this. The
VersionSet
should ideally represent the version history of a DB at a specific point in time, and should always be in a valid state, soCount
andAll
should not fail. The iterator to replaceAll
will have aValid
method, but should also not encounter an error.If a DB can't return a valid version history, it should return an error from the
Versions
method.Also, TMU this interface won't be exposed to SDK users. If the behavior changes, adding error return values should be a non-breaking change.
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.
Are you cashing versions when calling
Versions()
? If not, thenCount
can error if a connection to the DB will get broken in the meantime.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, versions are cached so that a
VersionSet
can refer to a self-contained version history state. If the number of saved versions is expected to be very large, we could change that, but then there's a race hazard - so we mustVersionSet
is closed, orDBConnection
interface, so they can be individually synchronized, orI don't see another option yet.