Remove Duplicated Strip Projects Code from SDKs #820
Conversation
mrzzy
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
mrzzy
changed the title
…se to specify project override
…ts do not interfere with otheer tests
…'s CachedSpecService
…e feature set Caused by passing entire get online request in unpackValueMap(), when in fact it should have been individual featureSetRequests. Caused features that was not in the first feature row to be nulled out.
| String.format("Unsupported feature reference: %s", featureRefString)); | ||
| } | ||
| throw new IllegalArgumentException( | ||
| String.format("Unsupported feature reference: %s", featureRefString)); |
There was a problem hiding this comment.
Can we be more specific with this exception?
| }); | ||
| return featureSetRequests; | ||
| } | ||
|
|
||
| /** Build a Feature Set request from the Feature Set specified by name and Feature References */ |
There was a problem hiding this comment.
A full Javadoc comment would be nice.
| FeatureSetSpec featureSetSpec; | ||
| try { | ||
| featureSetSpec = featureSetCache.get(featureSetName); | ||
| } catch (ExecutionException e) { |
There was a problem hiding this comment.
One of the major pain point that I have seen is when users are requesting features that dont exist because a store is not subscribed to it. It maybe out of scope for this PR, but is there a way that we can address that problem?
Basically they should get an exception that a feature set exists but is not being populated, instead of just "this feature or feature set doesnt exist".
There was a problem hiding this comment.
I think one way we can resolve this populate the feature set cache from core wholesale and apply the subscription checking only when request is made. This way we be able to determine if the request is rejected due missing feature set or because subscription was not applied properly.
…jectOverride parameter in docs
|
/lgtm |
|
/test test-end-to-end-batch |
1 similar comment
|
/test test-end-to-end-batch |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrzzy, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Problem
Continuation of: #693.
In v0.5 SDK, projects stripped from string Feature References in individual SDKs. This is a source of code duplication as the same strip project code is implemented and re implemented in the 3 SDKs. This duplication makes maintenance difficult as we have to maintain the same logic in all 3 SDKs.
What does the PR do
projecttoGetOnlineFeaturesRequestas optional field in ServingService.proto.projectwith project name, Serving get online features from the specifiedproject.GetOnlineFeaturesRequest'sprojectfield will take precedence overprojectspecified inFeatureReferencesif both are specified.GetOnlineFeatureRequest'sprojectfield.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: