Support of GC and S3 storages for registry in Java Feature Server #2043
Conversation
pyalex
requested review from
achals,
adchia,
felixwang9817,
tsotnet,
woop and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #2043 +/- ##
===========================================
+ Coverage 57.54% 83.31% +25.76%
===========================================
Files 100 100
Lines 8028 8080 +52
===========================================
+ Hits 4620 6732 +2112
+ Misses 3408 1348 -2060
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
feast-ci-bot
added
release-note
and removed
do-not-merge/release-note-label-needed
labels
pyalex
changed the title
pyalex
force-pushed
the
java-cloud-registry
branch
2 times, most recently
from
dd96ee2
to
85b166b
Compare
Signed-off-by: pyalex <[email protected]>
pyalex
force-pushed
the
java-cloud-registry
branch
from
85b166b
to
fa6d4fd
Compare
| this.registry = new Registry(this.registryFile.getContent()); | ||
|
|
||
| if (refreshIntervalSecs > 0) { | ||
| setupPeriodicalRefresh(refreshIntervalSecs); |
There was a problem hiding this comment.
In the AWS Lambda Feature server, every feast apply is responsible for tearing down existong lambdas and recreating them with the new registry file. Should we follow the same pattern in feast-serving? Just seems weird that the behavior for the two servings options is different in this respect.
There was a problem hiding this comment.
Although, I agree on consistency point, this is a little different case. Java Feature Server will be deployed in Kubernetes (see my proposal for deploying architecture in #2017 ) with Helm chart (at least for now). I also would argue that restarting pod on each registry change might be not be desired in production environment (not sure whether it's good thing for lambda as well, since it also doesn't start instantly).
But the main problem that currently feast sdk isn't capable of managing complex infrastructure, even aws lambda is currently deployed not completely (still requires manually configuration), and for this case we would need to manage kubernetes object. Probably that will be solved with introduction of pulumi.
But I still think that this current simple solution deserves to exist. It could be short-term solution until feast apply is capable of managing complex infra or long-term for people who will manage infra manually.
There was a problem hiding this comment.
Okay, yeah I think that's fair. For more "productionized" services, the ability to run without restarts is valuable.
There was a problem hiding this comment.
Is it worth making a TODO comment here with our reasoning? I just want to make sure that we capture our decision.
Signed-off-by: pyalex <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, pyalex 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 |
Signed-off-by: pyalex [email protected]
What this PR does / why we need it:
Previously only local file could be used as registry in Java Feature Server. This PR adds support for both Google Storage and AWS S3 as possible sources for registry file. Feature Server can only read registry file and not update it. It also may periodically check (when this enabled) if registry file was updated and load new version into memory.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: