refactor: Simplify fetcher interface #1746
Conversation
AndrewSisley
added
area/query
AndrewSisley
force-pushed
the
1589-fetcher-interface
branch
from
c1679c0 to
284b1b5
Compare
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1746 +/- ##
===========================================
+ Coverage 75.42% 75.68% +0.25%
===========================================
Files 208 208
Lines 21801 21735 -66
===========================================
+ Hits 16443 16448 +5
+ Misses 4210 4142 -68
+ Partials 1148 1145 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
db/fetcher/encoded_doc.go
Outdated
| schemaVersionID string | ||
| status client.DocumentStatus | ||
| properties map[client.FieldDescription]*encProperty | ||
| decodedpropertyCache map[client.FieldDescription]any |
There was a problem hiding this comment.
todo: change to decodePropertyCache
There was a problem hiding this comment.
Why? decode is a verb, and if there are values in here, this means it has been decoded (past-tense) (and I think the past-tense becomes an adjective, which the present tense cannot be)
There was a problem hiding this comment.
sorry I did a typo in my change request: decodedPropertyCache. I just want it to be camelcased properly.
There was a problem hiding this comment.
Ah 😆 I didnt spot that at all - thanks Fred, will change of course
- Fix casing
There was a problem hiding this comment.
Some questions and suggestions and one todo.
One notable question regarding the interface change was if we still want it to match the Enumerable interface?
Will run thru this again (went thru it all, but want to digest, and might have some other thoughts depending on the answer/approach to the above question)
But overall, very good!
db/fetcher/encoded_doc.go
Outdated
| // only get filter fields if filter=true | ||
| if onlyFilterProps && !prop.IsFilter { |
There was a problem hiding this comment.
todo: (This might be a problem from the original implementation) But I think the onlyFilterProps should precede the cached value check/assignment?
There was a problem hiding this comment.
I can see what you mean, but I thought this check was purely a performance thing from within this function - returning extra stuff would not be an issue, and if it is cached then the performance cost should be negligible?
Although, having written that out it still seems technically incorrect to return the value, even if there is no downside RE user visible behaviour (incl. perf.), I'll move it and see if anything breaks 😁
- Move
| @@ -514,14 +522,49 @@ func (df *DocumentFetcher) processKV(kv *keyValue) error { | |||
|
|
|||
| df.execInfo.FieldsFetched++ | |||
|
|
|||
| df.doc.Properties[fieldDesc] = property | |||
| df.doc.properties[fieldDesc] = property | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // FetchNext returns a raw binary encoded document. It iterates over all the relevant | |||
| // keypairs from the underlying store and constructs the document. | |||
| func (df *DocumentFetcher) FetchNext(ctx context.Context) (EncodedDocument, ExecInfo, error) { | |||
There was a problem hiding this comment.
question: Wasn't part of this refactor to make it also match the Enumerable interface? If so, I think it would be better to do this under a single PR rather than split, since its all very related.
There was a problem hiding this comment.
If this is the case, we can likely move the ExecInfo off the main Fetch method (it feels a little weird here anyway) and have a dedicated method to get the info instead.
There was a problem hiding this comment.
Wasn't part of this refactor to make it also match the Enumerable interface?
We for sure spoke a bit about that, and I started to look into it, but it felt slightly forced (due to a slight mismatch of funcs like Close and Reset, Init, Start, etc) , and had much less value IMO. This PR currently makes things much better, and cheaper for myself and Islam to build upon, changing to Enumerable would not speed up the Lens work, and I doubt it would help Islam.
We can look at making it Enumerable later I think, for now it is effort I would rather not spend, and would likely have very little impact on our productivity in the medium term.
we can likely move the ExecInfo off the main Fetch method (it feels a little weird here anyway) and have a dedicated method to get the info instead.
I also thought about doing that too, but it is not a new problem, and is pretty common to have stats like this outputted from funcs like this as return values. It is also extra effort now that does not benefit us anytime soon as far as I can see, and would only slow down the merge of changes that do benefit us right now.
There was a problem hiding this comment.
We for sure spoke a bit about that, and I started to look into it, but it felt slightly forced (due to a slight mismatch of funcs like Close and Reset, Init, Start, etc)
OK no problem. Just remembered talking about it and wanted to make sure it was addressed (regardless of in favor or against).
I also thought about doing that too, but it is not a new problem, and is pretty common to have stats like this outputted from funcs like this
Correct, this isnt a new problem, and not really all that important. The comment was only in relation to if we moved forward with the enumerable interface matching.
There was a problem hiding this comment.
Just remembered talking about it and wanted to make sure it was addressed
Yeah, I should probably have noted this in the PR description - I nearly did and then changed my mind as it wasnt noted in the related issue.
The comment was only in relation to if we moved forward with the enumerable interface matching.
Does sound like the cleanest way to handle it with an enumerable-derived interface :)
| @@ -183,162 +182,3 @@ func TestFetcherGetAllPrimaryIndexEncodedDocMultiple(t *testing.T) { | |||
| assert.NoError(t, err) | |||
| assert.NotNil(t, encdoc) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
just want to raise (my usual ;) ) objection to removing some of these unit tests.
There was a problem hiding this comment.
The function no longer exists, there is nothing to unit test. FetchNext is covered above.
There was a problem hiding this comment.
Theres two notable things here in these removed tests that arent covered in the tests that remain. 1) Decoding 2) Value checks. Both of these have value and can be either integrated in to the remaining tests, or we can keep the deleted ones and just update them to use the new APIs
There was a problem hiding this comment.
- Decoding
If we want unit test for decoding I think they should look quite different to these, and be far more comprehensive - the ones deleted only ever test a single string value, and one low, positive, integer. I don't think any time would really be saved by converting these tests to decoding unit tests.
- Value checks
That's weird, but you are right, the other tests never actually check the result of FetchNext, only that it doesn't error 🤷 - I still very much don't think they are worth keeping though, given that they are offering very little user protection vs our integration tests
AndrewSisley
force-pushed
the
1589-fetcher-interface
branch
from
284b1b5 to
407c867
Compare
And handle within the fetcher instance that fetches it. This means the status is now available via fetcher.FetchNext, an important step towards making the 3 fetcher functions equal.
Grants deleted doc support to all three FetchFoo funcs as the two others call FetchNext. All 3 funcs should now behave the same way, just outputing the result in a slightly different format.
As well as being inconsistent it is blockking the addition of a Properties() func
This allows a minor refactor, and provides access to the properties of the EncodedDoc from outside of the interfaced object.
Simplifies both the EncodedDoc interface, and the internals of the implementation.
FetchNextDoc and FetchNextDecoded now can just call FetchNext.
AndrewSisley
force-pushed
the
1589-fetcher-interface
branch
from
b2f9ab0 to
8c0b6eb
Compare
## 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).
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).
There is a fair amount going on here, suggest reviewing commit by commit - it should help explain what is going on and why.