GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat: Add read_from and client_az options to the Redis GLIDE client by NopeCharon · Pull Request #7093 · feast-dev/feast Conversation
Contributor
What this PR does / why we need it:The GLIDE client from #6870 always reads from the primary: the Feast config has no way to pass GLIDE's read strategy through, so clusters with replicas can't spread online reads across them. This adds two
online_store: type: redis redis_type: redis_cluster connection_string: "redis1:6379,redis2:6379" client: glide read_from: az_affinity client_az: us-east-1a I used dedicated options rather than The config is validated at load time, so a setting is never silently ignored:
Docs: Which issue(s) this PR fixes:Fixes #7086 Checks
Testing Strategy
Unit tests ( Manual test: I ran
All reads returned the written values. Misc
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## master #7093 +/- ## ========================================== + Coverage 51.79% 51.81% +0.01% ========================================== Files 458 458 Lines 57715 57735 +20 Branches 8483 8486 +3 ========================================== + Hits 29896 29915 +19 Misses 25793 25793 - Partials 2026 2027 +1
*This pull request uses carry forward flags. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@NopeCharon the PR is out-of-date, pls update it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Merged the latest master into the branch, no conflicts. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
ce7ce13 to
4b075f3
Compare
The GLIDE client added in feast-dev#6870 always read from the primary, because the Feast config had no way to pass GLIDE's read strategy through. Clusters with replicas could not spread online reads across them. Add two RedisOnlineStoreConfig options that map onto GLIDE's own settings: * read_from: primary (default), prefer_replica, az_affinity, az_affinity_replicas_and_primary or all_nodes, translated to glide_sync.ReadFrom. * client_az: the client's availability zone, used by the az_affinity strategies. The config is validated at load time so a setting can never be silently ignored: * read_from and client_az require client: glide, since redis-py does not use them. * A read_from other than primary requires redis_type: redis_cluster. A standalone GLIDE client sends every batch to the primary, so the batched HMGET read would ignore the strategy. * The az_affinity strategies require client_az, which GLIDE also enforces when building the client config. Verified against a Valkey 8 cluster with one primary and one replica: prefer_replica and az_affinity send the batched online_read to the replica, all_nodes spreads it over both nodes, and the default keeps reading from the primary. Fixes feast-dev#7086 Signed-off-by: NopeCharon <[email protected]>
4b075f3 to
38df0e8
Compare
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
feat: Glide Client should support reading from replicas
There was an error while loading. Please reload this page.