Add Support for Redis Cluster by lavkesh · Pull Request #502 · feast-dev/feast

lavkesh · 2020-02-27T13:38:32Z

This pull request adds REDIS_CLUSTER as a store.

Fixes #478

lavkesh · 2020-03-04T09:56:16Z

To test locally Create a dockerfile

Dockerfile

FROM maven:3.6-jdk-11
Add . /
CMD ./.prow/scripts/test-end-to-end-redis-cluster.sh

Build the project

mvn clean install -Dgpg.skip -DskipTests

Comment out these lines in the test-end-to-end-redis-cluster.sh

#.prow/scripts/download-maven-cache.sh \
#    --archive-uri gs://feast-templocation-kf-feast/.m2.2019-10-24.tar \
#    --output-dir /root/

# Build jars for Feast
#mvn --quiet --batch-mode --define skipTests=true clean package

Run

docker build -t end-to-end-redis-cluster  . 
docker run -it end-to-end-redis-cluster:latest

woop · 2020-03-05T05:30:11Z

ingestion/src/main/java/feast/ingestion/transform/WriteToStore.java

Is there code missing here?

No. It just falls through.
we call RedisCustomIO.write(getStore() for both the stores.

lavkesh · 2020-03-09T05:45:02Z

/retest

khorshuheng · 2020-03-25T03:09:11Z

/test test-end-to-end-batch

khorshuheng · 2020-03-25T03:22:07Z

https://github.com/gojek/feast/blob/2d45e9bce4c4f12486acd2ef74781e5028620456/ingestion/src/main/java/feast/ingestion/ImportJob.java#L119

This part needs to be changed as well, to include Redis Cluster as a valid store.

khorshuheng · 2020-04-14T04:25:20Z

/hold

khorshuheng · 2020-04-14T04:29:13Z

Resolved the merge conflict due to changes in storage API. Will be manually testing this.

Co-authored-by: Khor Shu Heng <khor.heng@gojek.com> Co-authored-by: Lavkesh Lahngir <lavkesh51@gmail.com>

khorshuheng · 2020-04-20T10:22:05Z

/unhold

woop · 2020-04-23T09:37:57Z

...c/main/java/feast/storage/connectors/rediscluster/retriever/RedisClusterOnlineRetriever.java

+                featureSetRequest.getFeatureReferences().asList());
+        featureRows.add(featureRowsForFeatureSet);
+      } catch (InvalidProtocolBufferException | ExecutionException e) {
+        throw Status.INTERNAL


Just wondering if we should be throwing gRPC exceptions so deep within the code base. Perhaps this should be left to higher layers?

While i agree that this should be left to higher layers, it is better to implemented that in a separate PR, as RedisOnlineRetriever will also be affected by the change.

woop · 2020-04-23T16:36:54Z

...cluster/src/main/java/feast/storage/connectors/rediscluster/writer/RedisClusterCustomIO.java

+            featureRows.forEach(row -> context.output(successfulInsertsTag, row));
+            featureRows.clear();
+          } catch (Exception e) {
+            featureRows.forEach(


Should we be logging to console or to statsd here if we have failures? or does that happen downstream?

It would be done downstream: https://github.com/gojek/feast/blob/master/ingestion/src/main/java/feast/ingestion/ImportJob.java#L166

feast-ci-bot · 2020-04-27T09:25:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavkesh, woop

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [woop]

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

woop · 2020-04-27T09:25:31Z

/lgtm

woop · 2020-04-27T09:25:39Z

Thank you @lavkesh and @khorshuheng

feast-ci-bot · 2020-04-27T09:26:17Z

@lavkesh: Updated the config configmap in namespace default at cluster default using the following files:

key config.yaml using file .prow/config.yaml

Details

In response to this:

This pull request adds REDIS_CLUSTER as a store.

Fixes #478

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.

lavkesh requested review from davidheryanto, khorshuheng, pradithya, woop and zhilingc as code owners February 27, 2020 13:38

lavkesh changed the title ~~Redis cluster store~~ [WIP] Redis cluster store Feb 27, 2020

feast-ci-bot added size/L do-not-merge/work-in-progress size/XL and removed size/L labels Feb 27, 2020

lavkesh changed the title ~~[WIP] Redis cluster store~~ Redis cluster store Feb 28, 2020

feast-ci-bot removed the do-not-merge/work-in-progress label Feb 28, 2020

lavkesh force-pushed the RedisClusterStore branch from f57624e to a0e7a07 Compare February 28, 2020 04:24

woop reviewed Mar 5, 2020

View reviewed changes

woop mentioned this pull request Mar 9, 2020

Feast 0.5 release #527

Closed

woop added this to the v0.5.0 milestone Mar 10, 2020

woop added area/serving area/storage kind/feature New feature or request labels Mar 10, 2020

woop changed the title ~~Redis cluster store~~ Add Support for Redis Cluster Mar 10, 2020

khorshuheng force-pushed the RedisClusterStore branch from f986cfc to 1afad9a Compare April 14, 2020 04:19

feast-ci-bot added size/XXL and removed size/XL labels Apr 14, 2020

feast-ci-bot added the do-not-merge/hold label Apr 14, 2020

Add Redis Cluster Support,

14f1431

Co-authored-by: Khor Shu Heng <khor.heng@gojek.com> Co-authored-by: Lavkesh Lahngir <lavkesh51@gmail.com>

khorshuheng force-pushed the RedisClusterStore branch from 1afad9a to 14f1431 Compare April 17, 2020 09:47

feast-ci-bot removed the do-not-merge/hold label Apr 20, 2020

woop reviewed Apr 23, 2020

View reviewed changes

ches mentioned this pull request Apr 26, 2020

Add computation and retrieval of batch feature statistics #612

Closed

woop approved these changes Apr 27, 2020

View reviewed changes

feast-ci-bot added the approved label Apr 27, 2020

feast-ci-bot assigned woop Apr 27, 2020

feast-ci-bot added the lgtm label Apr 27, 2020

feast-ci-bot merged commit 11151e8 into feast-dev:master Apr 27, 2020

terryyylim mentioned this pull request Apr 28, 2020

Fix redis cluster e2e #659

Merged

Conversation

lavkesh commented Feb 27, 2020 • edited by woop Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lavkesh commented Mar 4, 2020

To test locally Create a dockerfile

Dockerfile

Build the project

Comment out these lines in the test-end-to-end-redis-cluster.sh

Run

Uh oh!

woop Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

lavkesh Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

lavkesh commented Mar 9, 2020

Uh oh!

khorshuheng commented Mar 25, 2020

Uh oh!

khorshuheng commented Mar 25, 2020

Uh oh!

khorshuheng commented Apr 14, 2020

Uh oh!

khorshuheng commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khorshuheng commented Apr 20, 2020

Uh oh!

woop Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

khorshuheng Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

woop Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

khorshuheng Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Apr 27, 2020

Uh oh!

woop commented Apr 27, 2020

Uh oh!

woop commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feast-ci-bot commented Apr 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lavkesh commented Feb 27, 2020 •

edited by woop

Loading

khorshuheng commented Apr 14, 2020 •

edited

Loading

woop commented Apr 27, 2020 •

edited

Loading