Skip to content
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

Simplify Fetcher interface #1589

Closed
Tracked by #1002
AndrewSisley opened this issue Jun 26, 2023 · 2 comments · Fixed by #1746
Closed
Tracked by #1002

Simplify Fetcher interface #1589

AndrewSisley opened this issue Jun 26, 2023 · 2 comments · Fixed by #1746
Assignees
Labels
area/db-system Related to the core system related components of the DB refactor This issue specific to or requires *notable* refactoring of existing codebases and components

Comments

@AndrewSisley
Copy link
Contributor

AndrewSisley commented Jun 26, 2023

Currently we have:

FetchNext(ctx context.Context) (EncodedDocument, error)
FetchNextDecoded(ctx context.Context) (*client.Document, error)
FetchNextDoc(ctx context.Context, mapping *core.DocumentMapping) ([]byte, core.Doc, error)
  • FetchNext is never used.
  • FetchNextDecoded cannot return deleted items.
  • Nowhere in the codebase mix and matches (which would be really odd to do anyway), i.e. calls FetchNextDecoded and FetchNextDoc - is always one or the other.
  • FetchNextDecoded and FetchNextDoc are really just use-case transformations of fetcher output, not fetcher output itself.

This all makes it harder to maintain and use, particularly when layering stuff on top of it, like the Lens wrapper as it has to deal with 3 funcs that nearly do the same thing but not quite.

We can remove FetchNextDecoded and FetchNextDoc, they can sit as a layer on top (is just a stateless func call transforming the output of FetchNext). I am also in favour of changing it so that the Fetcher interface just extends the Enumerable interface. The transforms then just become enumerable.Selects, and stuff like Lens becomes super easy (todo in codebase, in the Lens fetcher).

Be careful with the time of this however. Many people are currently working in the same location.

@AndrewSisley AndrewSisley added area/db-system Related to the core system related components of the DB refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Jun 26, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.6 milestone Jun 26, 2023
@jsimnz
Copy link
Member

jsimnz commented Jun 26, 2023

Overall, im a big fan of this, less is more.

As for the enumerable interface implementation on the Fetcher side, what did you have in mind for the value of the type parameter, ie, what should the base Value method on the Fetcher return. Would EncodedDocument be too internal / diffucult to use in the context of Lens. My initial intuition would be that EncodedDocument should work well for the existin use cases that the fetcher is consumed by, but I dont yet have any intuition on the lens use cases regarding the fetcher.

As you mentioned, I think having the "decode" functions of FetchNextDecoded nd FetchNextDoc be moved off the interface into some utility functions in the fetcher package works great, as it allows whatever downstream consumer to use the representation they care about, without further complicating the fetcher.

@AndrewSisley
Copy link
Contributor Author

Noted in standup, but adding here for reference. I had EncodedDocument in mind (possible because that is what is already there), but have not given it too much thought, and currently have no strong feelings.

@fredcarle fredcarle modified the milestones: DefraDB v0.6, DefraDB v0.7 Jul 4, 2023
@AndrewSisley AndrewSisley self-assigned this Aug 1, 2023
AndrewSisley added a commit that referenced this issue Aug 3, 2023
## Relevant issue(s)

Resolves #1589

## Description

Simplifies the fetcher interface, removing 2 of the 3 FetchFoo
functions. As well as making it simpler, this also means that all (one)
of the functions have to behave in the same way (e.g. deleted doc
support).
shahzadlone pushed a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1589

## Description

Simplifies the fetcher interface, removing 2 of the 3 FetchFoo
functions. As well as making it simpler, this also means that all (one)
of the functions have to behave in the same way (e.g. deleted doc
support).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/db-system Related to the core system related components of the DB refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants