Implement get interface for FileDB #3286
Conversation
|
Code coverage for golang is |
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Add merge based on UpdatedAt field in case there are multiple parts of object are fetched.pipecd/pkg/datastore/filedb/filedb.go Lines 109 to 112 in e573321 This was created by todo plugin since "TODO:" was found in e573321 when #3286 was merged. cc: @khanhtc1202. |
khanhtc1202
changed the title
|
/todo skip |
|
Code coverage for golang is
|
pkg/datastore/filedb/filedb.go
Outdated
| } | ||
|
|
||
| if len(parts) == 1 { | ||
| return dataTo(parts[0], v) |
There was a problem hiding this comment.
I prefer to avoid using reflect package or copying data from another struct.
So how about unmarshal the data directly to v.
In the case of multiple shards, we can unmarshal the least significant one to v to merge.
There was a problem hiding this comment.
I feel you, if possible I would like to not use reflect as you said since it's slow and quite dangerous to do. 👍
In the case of multiple shards, we can unmarshal the least significant one to v to merge.
I get your point, but one thing I have to confirm. In case of multiple shards stored object, we may need to unmarshal once to get at least UpdatedAt field to enable comparisons between parts and choice which should be unmarshaled first. In such a case, we may have a flow of: unmarshal => compare => unmarshal again with unmarshaled data. It looks a bit odd but could avoid using reflect, if it is good for you too, I'm happy to adopt your suggestion 👍
There was a problem hiding this comment.
I think that UpdatedAt does not matter while merging them. UpdatedAt is for the whole entity, not each field so we can not rely on it to decide the value of which entity should be used.
I think each Collection we will need a Merge(from, to interface{}, fromShard, toShard Shard) function that merges the from one to the to one, the shard name should be passed to that Merge function as well to know the priority of their fields.
Btw, since this PR is only supporting 1 shard currently, so let's use JSON to unmarshal directly v at this time for the sake of simplicity.
And then talk more about the merging problem at another PR.
There was a problem hiding this comment.
Btw, since this PR is only supporting 1 shard currently, so let's use JSON to unmarshal directly v at this time for the sake of simplicity.
And then talk more about the merging problem at another PR.
okay, for sure 👍
There was a problem hiding this comment.
Thanks for the comment, addressed by a44fd83 🙏
|
Code coverage for golang is
|
|
Code coverage for golang is |
|
Nice. |
|
Here you go! |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: