Repository navigation
feat: Add read_from and client_az options to the Redis GLIDE client - #7093
Open
NopeCharon wants to merge 1 commit into
Open
NopeCharon wants to merge 1 commit into
NopeCharon wants to merge 1 commit into
Conversation
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]>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7093 +/- ##
==========================================
+ Coverage 50.97% 51.01% +0.03%
==========================================
Files 455 455
Lines 56994 57014 +20
Branches 8321 8324 +3
==========================================
+ Hits 29055 29083 +28
+ Misses 25954 25946 -8
Partials 1985 1985
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
RedisOnlineStoreConfigoptions that map directly onto GLIDE's own settings:read_from:primary(default),prefer_replica,az_affinity,az_affinity_replicas_and_primaryorall_nodes, translated toglide_sync.ReadFrom.client_az: the client's availability zone, used by theaz_affinitystrategies.I used dedicated options rather than
connection_stringparameters, as the issue suggests. The reason is thatconnection_stringparameters are also passed as kwargs to redis-py, which still handles writes and async reads, and redis-py would reject them.The config is validated at load time, so a setting is never silently ignored:
read_fromandclient_azrequireclient: glide, since redis-py doesn't use them.read_fromother thanprimaryrequiresredis_type: redis_cluster. A standalone GLIDE client sends every batch to the primary whateverread_fromis, so the batchedHMGETread would ignore the strategy. GLIDE's standaloneBatchOptionshas no routing option. I checked this against a real primary + replica: withPREFER_REPLICA, singlehmgetcalls went to the replica butexec(Batch(...))still went to the primary.az_affinitystrategies requireclient_az. GLIDE also enforces this, but only when the client is created.Docs:
docs/reference/online-stores/redis.mdgets a "Reading from replicas" section and two new rows in the configuration reference table.Which issue(s) this PR fixes:
Fixes #7086
Checks
git commit -s)Testing Strategy
Unit tests (
tests/unit/infra/online_store/test_redis.py) cover the default, the three validation rules, and the mapping of everyread_fromvalue ontoglide_sync.ReadFromandclient_az.Manual test: I ran
RedisOnlineStore.online_write_batch+online_readwithclient: glideagainst a Valkey 8 cluster with one primary (availability-zone zone-a) and one replica (zone-b). For each config I countedHMGETcalls per node fromINFO commandstatsover one read of 3 entities:primary)prefer_replicaall_nodesaz_affinity,client_az: zone-baz_affinity,client_az: zone-a(no replica in zone, falls back to other replicas)az_affinity_replicas_and_primary,client_az: zone-aAll reads returned the written values.
Misc