Add Support For Bigtable as Online Store#833
Add Support For Bigtable as Online Store#833Wirick wants to merge 29 commits intofeast-dev:masterfrom
Conversation
|
Hi @Wirick. Thanks for your PR. I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This comment has been minimized.
This comment has been minimized.
7a7c03e to
6be1913
Compare
|
/ok-to-test |
f5bb897 to
c4b9f36
Compare
1. Use basic copypasta from the redis implementation 2. Placeholder in place of final key manipulation logic 3. Wrestle with all the dependency issues
1. There are a number of things that I learned from maven so I went back and reexamined my assumptions. I also rebased and then forwardported some of the logic on a lateest master 2. BigtableIO seems similar to BigQueryIO in that it's not really exposed in a way to publish failed elements
1. Errors out on applying feature set with an inactiveRPC error, trying on master now
1. Validate that the bigquery pipeline still works for publishing features 2. Removed slf4j logger warnings by excluding the other versions
1. I don't know how to use a gax response observer correctly so I replaced readrowsasync with readrows, I don't think that it changes how the query is executed
1. Move the logic that was in the observer into the decoder 2. Make the structure conform to similar redis connector 3. Delete observer file 4. Latencies at 1k req/min are < 10ms
The feature reference list gets passed on decoder initialization in order to filter values before they are serialized if they are not needed in the request. Small latency improvment
maybe a better regex/set of regexes would be better
9c89095 to
3bffcd4
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chengcheng-pei, Wirick The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@Wirick: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
What this PR does / why we need it:
This implements support for using bigtable to ingest and serve low latency features, while having a fully managed and scalable database backend.
This design will be a jumping off point, as I see it as easily adaptable to different use cases of a service like bigtable. I would be interested in exploring if we could maybe have different couplings of serving and ingestion code so that we could do specific actions with keys. For our use case it may require a different bigtable instance, since it can't use a multi cluster routing app profile but that seems like a worthwhile investment. https://cloud.google.com/bigtable/docs/writes#increments
This way we could potentially have features that are incremented or appended to (like lists of values for computational/aggregation purposes) Certainly having to serialize lots of values may not be performant, but if we are careful about it I could see that we could compute aggregations on the fly. This may even be possible using the same sort of configurations as this implementation uses.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: