Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Redis and Redis Cluster #1511

Merged
merged 22 commits into from
Jun 9, 2021

Conversation

qooba
Copy link
Contributor

@qooba qooba commented Apr 27, 2021

What this PR does / why we need it:
PR adds Redis and RedisCluster support as online store. As described in #1497 it is backward compatible with Feast 0.9 thus it can be used for migration to 0.10

Which issue(s) this PR fixes:
Fixes #1497

Does this PR introduce a user-facing change?:

Redis and RedisCluster online stores support added. Format is compatible with Feast 0.9.

@feast-ci-bot
Copy link
Collaborator

Hi @qooba. 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.

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.

@woop
Copy link
Member

woop commented Apr 27, 2021

/ok-to-test

@woop
Copy link
Member

woop commented Apr 27, 2021

This is great work @qooba! Really surprised to see the implementation so quickly. We're still in the process of collecting feedback on the #1497, given that we may need to support a more scalable ingestion API for real time events. We may need a little bit of time before we can review and merge this code in, but we're super thankful for the contribution!!

@woop
Copy link
Member

woop commented Apr 28, 2021

One thing that stands out is that we don't have any automated tests for Redis as part of this PR. We'll definitely need to extend the PR to also include an integration test. @qooba do you want to take a stab at that?

One option is https://github.com/testcontainers/testcontainers-python, another is to just mark the tests as integration tests using @pytest.mark.integration and then only run tests on GitHub Actions with a Redis service provisioned

@qooba
Copy link
Contributor Author

qooba commented Apr 28, 2021

Sure @woop I will extend PR with integration tests.

@qooba qooba requested a review from a team as a code owner April 28, 2021 23:58
@qooba
Copy link
Contributor Author

qooba commented Apr 29, 2021

/retest

@woop
Copy link
Member

woop commented Apr 30, 2021

For some reason it's not allowing you to run tests until you've actually merged some code into Feast. I'll need to approve every time.

Copy link
Collaborator

@jklegar jklegar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @qooba! Yep we'll want integration tests to make sure these methods work in the same way as their GCP equivalents and don't rot in the future. For integration tests, adding a sdk/python/tests/test_cli_redis.py that is the equivalent of sdk/python/tests/test_cli_gcp.py should cover the main parts, plus adding redis into the GitHub actions workflows like Willem said above

@qooba
Copy link
Contributor Author

qooba commented May 6, 2021

@jklegar I have added sdk/python/tests/test_cli_redis.py and also sdk/python/tests/test_offline_online_store_consistency.py::test_redis_offline_online_store_consistency to check offline consistency with redis. Redis service also added to GitHub action workflow.

@qooba
Copy link
Contributor Author

qooba commented May 6, 2021

The test sdk/python/tests/test_cli_redis.py::test_basic fails because added configuration is not used:

env:
  REDIS_TYPE: REDIS
  REDIS_CONNECTION_STRING: redis:6379,db=0

@jklegar can you help with this ?

@jklegar
Copy link
Collaborator

jklegar commented May 10, 2021

@qooba I've added the redis service to pr_integration_tests.yml in master, can you rebase this diff? Once you rebase it'll run the updated action here

@qooba
Copy link
Contributor Author

qooba commented May 12, 2021

@jklegar there will be small change in github action configuration:
b8b0d9f

Additionally macOS-latest doesn't have docker thus tests will never pass can you remove it :)

And commit once again to master then I will rebase.

@woop woop changed the title Feature/online redis Add support for Redis and Redis Cluster May 13, 2021
@jklegar
Copy link
Collaborator

jklegar commented May 18, 2021

Thanks @qooba and apologies for the delay here, it should be good to rebase now

@qooba
Copy link
Contributor Author

qooba commented May 18, 2021

Thanks @jklegar rebased and tests are passing now :)

@woop
Copy link
Member

woop commented May 24, 2021

/kind feature

@feast-ci-bot feast-ci-bot added the kind/feature New feature or request label May 24, 2021
qooba and others added 22 commits June 9, 2021 12:52
Signed-off-by: Willem Pienaar <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Copy link
Member

woop commented Jun 9, 2021

/lgtm

@feast-ci-bot feast-ci-bot merged commit 0d7e858 into feast-dev:master Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Redis to new Feast
6 participants