feat: add config metadata to object store results #75
Conversation
breaking change for object store apis
aaron-steinfeld
requested review from
findingrish,
surajpuvvada and
saxenakshitiz
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #75 +/- ##
============================================
+ Coverage 84.27% 85.15% +0.87%
- Complexity 286 305 +19
============================================
Files 36 38 +2
Lines 1418 1495 +77
Branches 45 45
============================================
+ Hits 1195 1273 +78
+ Misses 194 191 -3
- Partials 29 31 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
|
||
| public Optional<T> getObject(RequestContext context) { | ||
| public Optional<T> getData(RequestContext context) { |
There was a problem hiding this comment.
The caller may require creation timestamp in response of get call. Can we just return same ConfigObject here as well?
There was a problem hiding this comment.
Not without introducing new grpc APIs - the original design of config service expected much more hierarchical config usage, so get (unlike upsert/delete/getAll) takes in any number of contexts and returns a merged result. That merged result has no associated context or metadata, since it may be a merge of multiple entries.
If we do have the need for timestamp on the singular get, we can introduce a getByContext rpc or something similar and then update this.
There was a problem hiding this comment.
Got it. I have never understood the merge usecase. However I agree, we can add another get call to return the data with timestamps. Can we add that call now or if need arises?
There was a problem hiding this comment.
I'd wait until the need arises, since it's purely additive to what we have here.
The merge use case is, for example - imagine fetching a config for an api where we may have some tenant-wide config, some service level config, and some api level config. We'd want to return a merged result conceptually like:
Config getConfigForApi() {
return apiConfig.withDefaultValuesFrom(serviceConfig.withDefaultValuesFrom(tenantConfig));
}
This comment has been minimized.
This comment has been minimized.
Description
This change updates the object store abstractions to return more metadata with results such as timestamps (and eventually could include things like version, last updated by etc.). These are stored and returned, but not exposed currently. This includes a clarifying terminology change, referring to the raw config as "data" while the full config with metadata as "object". Open to other names if anyone has suggestions.
Note: I did this as a breaking change for object store apis to avoid duplicate functionality, which is acceptable as this is not a published artifact.
Testing
Added and updated UTs. Ran Integration tests.
Checklist: