refactor: Datastore interfaces replaced with corekv #3298
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
Currently focusing on some low level structures/APIs such as the basic getter/setter interface changes, handling keys that are now `[]byte` instead of `ds.Key`, and a few other smaller items. Haven't started the tests, fetcher, or any calls to the old `Query` API (need to be migrated to the new Iterator API).
This was a "plain reading" refactor to replace the query API with the Iterator API. There may need to be different behavior for some reason (I don't think so, but maybe). The one thing to note that is different is that creating an Iterator doesn't return an error, and there is no `result.Error` value per iterator. The reason for the latter is that the old query.Query batched/collected a lot of results before the user actually asks for it, so it needs to collect the errors as well. Since this is a simpler iterator, there is no need for that. Addtionally, there may be a few call sites that don't close the iterator/query from before. I've updated a few that I noticed, but there may be a few remaining. THIS IS A BUG if they aren't closed.
With the new design, we get to remove the dual Iterator structure (the first isn't really an iterator, but a handle to create iterators). Now there is just a single iterator `kvIter`. We also don't need the previous complex `Iterable` interface that was defined in the `datastore` package. The corekv Iterator natively supports both Prefix and Range (start, end) iteration controlled by the `corekv.IterOptions`. Again, this is a "plain text" refactor without assuming anything.
NOTE: There is a filter in the indexer iterator file that hasn't been included, again focusing on the "plain text" refactoring.
jsimnz
added
area/datastore
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Relevant issue(s)
Related to #3275 #1813
Description
This PR is ONLY as a reference for #3275 on going by Andy. This refactor is based on a rather old (back in February ~v0.9) version of Defra. There hasn't been many changes to the
datastorepackage since then, but there has been a few changes to thefetcherrefactor, beyond the notable #3277 open PR.This Draft PR should be able to clearly demonstrate the design approach for how to integrate corekv, how we can delete almost all of the
datastorepackage.What isn't shown in this PR that is a win for replacing
go-datatsorewithcorekvis just how much more simple the internals of thecorekv.Iteratoris in comparison to theds.Query, not just in API, but internals of the how the Query system actual is evaluated.Initial refactor to integrate corekv into the codebase, focusing on two goals
SeekAPI in thefetcher.DocumentFetcherdesign to enable early filter optimizationNote: The 2nd goal isn't actually implemented, work was started recently on converting the fetcher seek functionality into this refactor branch, but considering the current open PR for refactoring the fetcher #3277, and the delay to integrate the filter-seek optimization until it is formally verified to be more efficient, i'll leave that work out of this branch.
Additionally, this refactor should be reviewed as a "plain text" refactor. Which means we're not aiming to optimize or redesign, soley to swap datastore interfaces and implementations with the least number of semantic differences.
How has this been tested?
IT HASN'T, it isn't finished to actually be compilable and therefore testable.
Notes for Reviewers (Andy ;) )
The commits are in a farily organized and chronological order of operations for the refactor. Some of the commits have some extra context that might help understand reasons.