Add merge shards data for multiple shards stored object #3316
Conversation
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
nghialv
left a comment
nghialv
left a comment
There was a problem hiding this comment.
Look neat!
Just some nits, please check them.
| return json.Unmarshal(parts[0], v) | ||
| } | ||
|
|
||
| // TODO: Add merge based on UpdatedAt field in case there are multiple parts of object are fetched. |
There was a problem hiding this comment.
Please close this issue once this PR got merged.
There was a problem hiding this comment.
there is no issue like that, since I used the /todo skip command last time 🙏
There was a problem hiding this comment.
Thanks for your confirmation. 👍
pkg/datastore/filedb/codec.go
Outdated
| } | ||
| } | ||
| // TODO: Find a better way to set value of field UpdatedAt. | ||
| reflect.ValueOf(e).Elem().FieldByName("UpdatedAt").SetInt(latest) |
There was a problem hiding this comment.
We can also define a updatedAtSetter interface beside updatedAtGetter as above to cast and set.
We just use this reflect magic in case it is not castable.
There was a problem hiding this comment.
in that case, we have to implement SetUpdatedAt for all models struct since the code generated by protobjf does not contain that function, which a bit odd to me, is that okay?
ref: golang/protobuf#65
There was a problem hiding this comment.
in that case, we have to implement SetUpdatedAt for all models struct since the code generated by protobjf does not contain that function
Yes, I see. I think that is fine.
There was a problem hiding this comment.
But you can note that way to your TODO and deal with that later.
We need a workable version first before a perfect one.
Co-authored-by: Le Van Nghia <[email protected]>
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Find a better way to set value of field UpdatedAt.pipecd/pkg/datastore/filedb/codec.go Lines 62 to 65 in 953b0e0 This was created by todo plugin since "TODO:" was found in 953b0e0 when #3316 was merged. cc: @khanhtc1202. |
|
The following files are not gofmt-ed. By commenting pkg/datastore/filedb/codec.go--- pkg/datastore/filedb/codec.go.orig
+++ pkg/datastore/filedb/codec.go
@@ -29,12 +29,12 @@
if ok {
return dcol.Decode(e, parts...)
}
-
+
// In case it's single part contained object, unmarshal it directly.
if len(parts) == 1 {
return json.Unmarshal(parts[0], e)
}
-
+
return merge(e, parts...)
}
|
|
Code coverage for golang is
|
|
The following files are not gofmt-ed. By commenting pkg/datastore/filedb/codec.go--- pkg/datastore/filedb/codec.go.orig
+++ pkg/datastore/filedb/codec.go
@@ -28,12 +28,12 @@
if ok {
return dcol.Decode(e, parts...)
}
-
+
// In case it's single part contained object, unmarshal it directly.
if len(parts) == 1 {
return json.Unmarshal(parts[0], e)
}
-
+
return merge(e, parts...)
}
|
|
/golinter fmt |
|
Looks neat! |
|
Pretty good! |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR introduces a way to merge raw data for multiple shards stored objects. I defined a new interface
ShardDecoder, which will be used in case of building object from fetched data in filedb.The PR also contains the implementation of the
ShardDecoderinterface for onlycommandCollectionsince the Command model is the only model which is multiple shards stored and there is a collision on its field namedStatus(should be stored in 2 shards: agent and piped)Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: