Set TTL on Redis values by ravdin · Pull Request #955 · feast-dev/feast

ravdin · 2020-08-21T20:52:51Z

Background

Currently no TTL is set on values stored in Redis. This results in the database storing stale values that are never referenced.

Solution

This change allows an import job to be configured to set the TTL on Redis values based on the FeatureSet's max age. If the TTL is less than or equal to zero, the value will not be written to Redis. The configuration settings are:

enableRedisTTL: Set to true to enable setting the Redis TTL. Defaults to false.
maxRedisTTLJitterSeconds: Adds a random value to the TTL, up to this value. Defaults to 0.

#555

feast-ci-bot · 2020-08-21T20:53:06Z

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

woop · 2020-08-24T09:11:44Z

/ok-to-test

woop · 2020-09-02T11:22:27Z

Thanks for the contribution @ravdin

So our summary on the conversation from the community call

Max age for TTL makes sense
You need to move the Redis specific config into the Redis or RedisCluster store configuration protos
We should have a maximum TTL. If the user does not have a valid max age, or if the max age is larger than the maximum, then we use the maximum. This is also configured in the Redis configuration.

Does that make sense?

ravdin · 2020-09-02T23:14:27Z

@woop @pyalex I added a maximum Redis TTL and two unit tests (test that we're using the maximum if there is no max age and confirm that we're overriding the max age with the configured maximum). If the maximum is set to 0 (i.e. not set), then the behavior will be as if there is no maximum.

Can you confirm that we want to override the feature set max age with this maximum if the max age is larger? I'm not sure that I see the use case for this.

woop · 2020-09-03T06:11:42Z

maybe we should specify that this is in seconds?

woop · 2020-09-03T06:16:13Z

Would you mind fixing the comments. Not sure why its broken up.

woop · 2020-09-03T06:16:33Z

Would be great if this was JavaDoc style comments

pyalex · 2020-09-24T02:59:11Z

/retest test-end-to-end-batch-dataflow

pyalex · 2020-09-24T06:13:35Z

/retest test-end-to-end-batch-dataflow

pyalex · 2020-09-24T09:25:10Z

/retest test-end-to-end-batch-dataflow

* Add entity data generator * Update core protos * Add entities as higher-level concept * Update python sdk * Add tests * Update to use scalar entity * Address PR comments * Update proto and tests * Remove redis pytest * Remove wildcard filters * Update golang generated files * Update tests * Rename constraints * Update list entities method * Update go protos

* Reorganise existing protos in CoreService by type. Signed-off-by: Terence <terencelimxp@gmail.com> * Add new FeatureTables API to Core Protobuf definitions Signed-off-by: Terence <terencelimxp@gmail.com> * Fix name collision in proto java outer classname with message name Signed-off-by: Terence <terencelimxp@gmail.com> * Add missing max age field to Feature Table proto. Signed-off-by: Terence <terencelimxp@gmail.com> * Add Flyway DB migration to add Feature Table API. Signed-off-by: Terence <terencelimxp@gmail.com> * Rename options field to options_json and change type to text. * Options to be stored as Protobuf JSON. * Change from varchar to text to remove char limit Signed-off-by: Terence <terencelimxp@gmail.com> * FeatureTable: Rename entity_names to entities Signed-off-by: Terence <terencelimxp@gmail.com> * Revert Reorganise existing protos in CoreService by type as it make it hard for reviewers to review changes Signed-off-by: Terence <terencelimxp@gmail.com> * Add FeatureSource entity for native representation of FeatureSource protobuf Signed-off-by: Terence <terencelimxp@gmail.com> * Add missing nullable annotation on FeatureSource entity. Signed-off-by: Terence <terencelimxp@gmail.com> * Update ListFeatureTablesRequest's Filter to follow naming convention. Signed-off-by: Terence <terencelimxp@gmail.com> * Add missing serialization code for FeatureSource's field mapping. Signed-off-by: Terence <terencelimxp@gmail.com> * Split Feature proto from FeatureTable proto. Signed-off-by: Terence <terencelimxp@gmail.com> * Update FeatureTable entity_names field to entities Signed-off-by: Terence <terencelimxp@gmail.com> * Revert putting project in feature table spec Signed-off-by: Terence <terencelimxp@gmail.com> * Update ListFeatureTable Proto to return full FeatureTable objects and limit to listing from one Project. Signed-off-by: Terence <terencelimxp@gmail.com> * Fix typo in CoreService proto Signed-off-by: Terence <terencelimxp@gmail.com> * Add FeatureV2 core model to store FeatureSpecV2 Signed-off-by: Terence <terencelimxp@gmail.com> * Add FeatureTable core model to store FeatureTable protos Signed-off-by: Terence <terencelimxp@gmail.com> * Fix naming grammar in CoreService proto Signed-off-by: Terence <terencelimxp@gmail.com> * Standardise naming of specifying projects in CoreService proto Signed-off-by: Terence <terencelimxp@gmail.com> * Rename FeatureSource proto to FeatureSourceSpec for compatiblity. Signed-off-by: Terence <terencelimxp@gmail.com> * Update FeatureSource model to store type specific options as seperate columns instead of JSON. Signed-off-by: Terence <terencelimxp@gmail.com> * Add FeatureTableTest unit test to test FeatureTable core model Signed-off-by: Terence <terencelimxp@gmail.com> * Add FeatureTableValidator to validate FeatureTableSpec protobufs Signed-off-by: Terence <terencelimxp@gmail.com> * Add listFeatureTables(), applyFeatureTable() & getFeatureTable() to Core's SpecService Signed-off-by: Terence <terencelimxp@gmail.com> * Add FeatureTableRepository to save & retrieve FeatureTables in database. Signed-off-by: Terence <terencelimxp@gmail.com> * Fix hibernate errors on Feast Core boot. Signed-off-by: Terence <terencelimxp@gmail.com> * Implement listFeatureTables() , applyFeatureTable(), and getFeatureTable() in CoreServiceImpl Signed-off-by: Terence <terencelimxp@gmail.com> * Add applyFeatureSet integration tests SpecServiceIT Signed-off-by: Terence <terencelimxp@gmail.com> * Various fixes for creating FeatureTabes with applyFeatureTable Signed-off-by: Terence <terencelimxp@gmail.com> * Fixed bug with updating FeatureTable Signed-off-by: Terence <terencelimxp@gmail.com> * Update ListFeatureTables Signed-off-by: Terence <terencelimxp@gmail.com> * Add Python SDK Signed-off-by: Terence <terencelimxp@gmail.com> * Update GetFeatureTable Signed-off-by: Terence <terencelimxp@gmail.com> * Remove unused proto imports and generate go protos Signed-off-by: Terence <terencelimxp@gmail.com> * Fix ListFeatureTables IT Signed-off-by: Terence <terencelimxp@gmail.com> * Update comment to generalize FeatureSource's field mapping to all fields instead of just for feature. Signed-off-by: Terence <terencelimxp@gmail.com> * Fix feature table validator condition Signed-off-by: Terence <terencelimxp@gmail.com> * Fix feature table unit tests Signed-off-by: Terence <terencelimxp@gmail.com> * Update feature source proto Signed-off-by: Terence <terencelimxp@gmail.com> * Address PR comments Signed-off-by: Terence <terencelimxp@gmail.com> * Replace test with IT Signed-off-by: Terence <terencelimxp@gmail.com> * Update IT config Signed-off-by: Terence <terencelimxp@gmail.com> * Fix removal of entity check Signed-off-by: Terence <terencelimxp@gmail.com> * Fix test sort issue Signed-off-by: Terence <terencelimxp@gmail.com> * Store source options as json Signed-off-by: Terence <terencelimxp@gmail.com> * Update go protos Signed-off-by: Terence <terencelimxp@gmail.com> * Remove go FeatureSource proto Signed-off-by: Terence <terencelimxp@gmail.com> * Increase IT max pool size Signed-off-by: Terence <terencelimxp@gmail.com> * Reduce pool size instead Signed-off-by: Terence <terencelimxp@gmail.com> * Replace mutablemapping with dict Signed-off-by: Terence <terencelimxp@gmail.com> * Standardize use of timestamp_column instead of ts_column Signed-off-by: Terence <terencelimxp@gmail.com> Co-authored-by: Terence Lim <terencelimxp@gmail.com>

Signed-off-by: Terence <terencelimxp@gmail.com>

pyalex · 2020-10-05T09:46:31Z

/test test-end-to-end-batch-dataflow

* test scala spark Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * offline batch ingestion in spark Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * clean up Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * deduplicate rows & use latest Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * clarify Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * validation & deadletter Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * tests on mapping & deadletter Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * scala styling Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * integration test stage Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * remove version from ingestion-spark pom Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * refactor job options Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * clean up dependencies + some api docs Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * extend mapping test Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * add shade plugin version & group Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * disable buildkit on docker build Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com>

* stable job name in row metrics Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * fix test Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com> * fix test Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com>

…ev#1025) Signed-off-by: Oleg Avdeev <oleg.v.avdeev@gmail.com>

* Pyspark job for feature batch retrieval Signed-off-by: Khor Shu Heng <khor.heng@gojek.com> * Add pyspark to ci requirements Signed-off-by: Khor Shu Heng <khor.heng@gojek.com> * Additional documentation and col mapping Signed-off-by: Khor Shu Heng <khor.heng@gojek.com> * Add Schema validation Signed-off-by: Khor Shu Heng <khor.heng@gojek.com> * Improve test case and documentation Signed-off-by: Khor Shu Heng <khor.heng@gojek.com> * Change max age to integer, filter source feature tables, tests for large dataframe Signed-off-by: Khor Shu Heng <khor.heng@gojek.com> Co-authored-by: Khor Shu Heng <khor.heng@gojek.com>

ravdin · 2020-10-08T15:55:18Z

@pyalex I did a rebase on the branch (no other changes though). Do you have timing on when you're looking to merge?

pyalex · 2020-10-14T06:24:16Z

@ravdin can you please fix this DCO check? I guess this is only thing which blocks merge

See merge request personalization/mlp/feature-engineering/feast!17 Fixed formatting issues Fixed complile errors Fixed unit tests Added legal comments Revert "Added legal comments" This reverts commit dd85eec177d5b0aa1c0576a55a2208c3ae3321b5. Fix for redis feature sink implementation Restored function to original location Formatting Moved redis TTL settings to store proto Added max redis TTL Code review fixes Resolved merge conflicts Reverted ImportOptions Refactor of redis TTL calculations Refactor of redis TTL calculations Code review fixes Signed-off-by: Bill Ravdin <bravdin@zulily.com>

ravdin · 2020-10-14T18:47:36Z

@pyalex Done.

pyalex · 2020-10-16T03:28:00Z

/lgtm

woop · 2020-11-02T03:16:34Z

@ravdin can we retarget this to 0.7 please? (switching it now)

Edit: Seems like this broke the history. For 0.8 we are moving to Spark, so a lot of our code has changed. I'd love to get this functionality merged in still.

Should we hold off on it until 0.8 is cut so that it's easier to do?

feast-ci-bot · 2021-01-28T15:43:16Z

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

Test name	Commit	Details	Rerun command
test-end-to-end-batch-dataflow	3e5bb92c403c53f5d866b4d22957f29baf647730	link	`/test test-end-to-end-batch-dataflow`
test-end-to-end-auth	`c994197`	link	`/test test-end-to-end-auth`
test-end-to-end	`c994197`	link	`/test test-end-to-end`
python-sdk-integration-test	`c994197`	link	`/test python-sdk-integration-test`
test-end-to-end-aws	`c994197`	link	`/test test-end-to-end-aws`
test-end-to-end-gcp	`c994197`	link	`/test test-end-to-end-gcp`
test-end-to-end-sparkop	`c994197`	link	`/test test-end-to-end-sparkop`
test-end-to-end-azure	`c994197`	link	`/test test-end-to-end-azure`
test-telemetry	`c994197`	link	`/test test-telemetry`

Full PR test history. Your PR dashboard.

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.

woop · 2021-02-07T23:54:35Z

Closing this PR for the time being. @ravdin please let me know if we should reopen it or target a different branch/version.

ravdin requested review from davidheryanto, khorshuheng, pyalex, woop and zhilingc as code owners August 21, 2020 20:52

feast-ci-bot added needs-kind size/XL labels Aug 21, 2020

feast-ci-bot added the needs-ok-to-test label Aug 21, 2020

ravdin force-pushed the ravdin/redis-ttl branch 3 times, most recently from f5bca33 to 69bca21 Compare August 21, 2020 21:08

feast-ci-bot added ok-to-test and removed needs-ok-to-test labels Aug 24, 2020

woop added the kind/techdebt label Aug 24, 2020

feast-ci-bot removed the needs-kind label Aug 24, 2020

woop added kind/feature New feature or request needs-kind labels Aug 24, 2020

feast-ci-bot removed the needs-kind label Aug 24, 2020

pyalex reviewed Aug 27, 2020

View reviewed changes

Comment thread ingestion/src/main/java/feast/ingestion/ImportJob.java Outdated

ravdin force-pushed the ravdin/redis-ttl branch from 69bca21 to 3d417f6 Compare September 1, 2020 17:04

woop reviewed Sep 2, 2020

View reviewed changes

Comment thread storage/connectors/redis/src/main/java/feast/storage/connectors/redis/writer/RedisCustomIO.java Outdated

ravdin force-pushed the ravdin/redis-ttl branch 2 times, most recently from 95e4414 to 6b02826 Compare September 2, 2020 23:10

woop reviewed Sep 3, 2020

View reviewed changes

Comment thread protos/feast/core/Store.proto Outdated

woop Sep 3, 2020

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

maybe we should specify that this is in seconds?

woop reviewed Sep 3, 2020

View reviewed changes

feast-ci-bot added the approved label Sep 24, 2020

woop and others added 4 commits September 26, 2020 05:31

GitBook: [master] 37 pages modified

dbd8b71

Update prow config and makefile (feast-dev#1024)

8bf55de

Signed-off-by: Terence <terencelimxp@gmail.com>

pyalex and others added 4 commits October 6, 2020 13:26

add redis SSL support to the offline-to-online ingestion job (feast-d…

61133e4

…ev#1025) Signed-off-by: Oleg Avdeev <oleg.v.avdeev@gmail.com>

ravdin force-pushed the ravdin/redis-ttl branch from 3e5bb92 to b230ba8 Compare October 7, 2020 23:52

ravdin force-pushed the ravdin/redis-ttl branch from b230ba8 to c994197 Compare October 14, 2020 17:51

feast-ci-bot assigned pyalex Oct 16, 2020

feast-ci-bot added the lgtm label Oct 16, 2020

woop changed the base branch from master to v0.7-branch November 2, 2020 03:17

feast-ci-bot added size/XXL and removed size/XL labels Nov 2, 2020

woop mentioned this pull request Nov 18, 2020

Expires Redis Keys based on Feature Table Max Age #1161

Merged

woop closed this Feb 7, 2021

Uh oh!

Conversation

ravdin commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Solution

Uh oh!

feast-ci-bot commented Aug 21, 2020

Uh oh!

woop commented Aug 24, 2020

Uh oh!

Uh oh!

Uh oh!

woop commented Sep 2, 2020

Uh oh!

ravdin commented Sep 2, 2020

Uh oh!

woop Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

woop Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

woop Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

pyalex commented Sep 24, 2020

Uh oh!

pyalex commented Sep 24, 2020

Uh oh!

pyalex commented Sep 24, 2020

Uh oh!

pyalex commented Oct 5, 2020

Uh oh!

ravdin commented Oct 8, 2020

Uh oh!

pyalex commented Oct 14, 2020

Uh oh!

ravdin commented Oct 14, 2020

Uh oh!

pyalex commented Oct 16, 2020

Uh oh!

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

Uh oh!

Uh oh!

feast-ci-bot commented Jan 28, 2021

Uh oh!

woop commented Feb 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ravdin commented Aug 21, 2020 •

edited

Loading

woop commented Nov 2, 2020 •

edited

Loading