Skip to content

Add Support For Bigtable as Online Store#833

Closed
Wirick wants to merge 29 commits intofeast-dev:masterfrom
postmates:postmates/add_bigtable_store
Closed

Add Support For Bigtable as Online Store#833
Wirick wants to merge 29 commits intofeast-dev:masterfrom
postmates:postmates/add_bigtable_store

Conversation

@Wirick
Copy link
Copy Markdown
Contributor

@Wirick Wirick commented Jun 29, 2020

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?:

Add Bigtable Store

@feast-ci-bot
Copy link
Copy Markdown
Collaborator

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

@Wirick

This comment has been minimized.

@Wirick Wirick force-pushed the postmates/add_bigtable_store branch from 7a7c03e to 6be1913 Compare June 30, 2020 11:24
@Wirick Wirick mentioned this pull request Jul 1, 2020
@woop
Copy link
Copy Markdown
Member

woop commented Jul 9, 2020

/ok-to-test

@Wirick Wirick force-pushed the postmates/add_bigtable_store branch from f5bb897 to c4b9f36 Compare July 20, 2020 20:13
Wirick added 13 commits July 22, 2020 04:14
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
@Wirick Wirick force-pushed the postmates/add_bigtable_store branch from 9c89095 to 3bffcd4 Compare July 22, 2020 11:14
@feast-ci-bot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chengcheng-pei, Wirick
To complete the pull request process, please assign zhilingc
You can assign the PR to them by writing /assign @zhilingc in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot
Copy link
Copy Markdown
Collaborator

@Wirick: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
publish-docker-images b9a95e9 link /test publish-docker-images
test-end-to-end-auth 803f12a link /test test-end-to-end-auth
test-end-to-end-batch-dataflow 803f12a link /test test-end-to-end-batch-dataflow
test-end-to-end-batch 803f12a link /test test-end-to-end-batch

Full PR test history

Details

Instructions 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.

@stale
Copy link
Copy Markdown

stale bot commented Oct 13, 2020

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.

@stale stale bot added the wontfix This will not be worked on label Oct 13, 2020
@stale stale bot closed this Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants