feat: add IBigSegmentStore interface + Redis and DynamoDB stores#536
feat: add IBigSegmentStore interface + Redis and DynamoDB stores#536beekld wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0082f6e. Configure here.
| return std::unique_ptr<RedisBigSegmentStore>(new RedisBigSegmentStore( | ||
| std::make_unique<sw::redis::Redis>(std::move(uri)), | ||
| std::move(prefix))); | ||
| } catch (sw::redis::Error const& e) { |
There was a problem hiding this comment.
Should we catch std::exception as well?
| public: | ||
| DynamoDBBigSegmentTests() | ||
| : table_name_("ld-dynamodb-big-segments-test"), | ||
| prefix_("testprefix"), |
There was a problem hiding this comment.
Should we have a test for an empty prefix?
| std::make_unique<sw::redis::Redis>(std::move(uri)), | ||
| std::move(prefix))); | ||
| } catch (sw::redis::Error const& e) { | ||
| return tl::make_unexpected(e.what()); |
There was a problem hiding this comment.
Do we need to sanitize this for the redis user/password?
There was a problem hiding this comment.
Or maybe make this and the dynamo version into something vague? Main concern is just a URI coming out which includes user/password or something else the customer puts into the URI and considers secret.
| IBigSegmentStore::GetMembershipResult RedisBigSegmentStore::GetMembership( | ||
| std::string const& context_hash) const { | ||
| std::string const include_key = | ||
| prefix_ + ":" + kIncludeKeyNamespace + ":" + context_hash; |
There was a problem hiding this comment.
nit: Would it be work it to cache prefix_ + ":" + kIncludeKeyNamespace and prefix_ + ":" + kExcludeKeyNamespace then just add on the context_hash?
| std::string prefix) | ||
| : redis_(std::move(redis)), | ||
| prefix_(std::move(prefix)), | ||
| sync_time_key_(prefix_ + ":" + kSyncTimeKey) {} |
There was a problem hiding this comment.
Should we check for an empty prefix?
.Net and JS do, but Java doesn't.
public RedisStoreBuilder<T> Prefix(string prefix)
{
_prefix = string.IsNullOrEmpty(prefix) ? Redis.DefaultPrefix : prefix;
return this;
}|
|
||
| auto const it = item.find(kBigSegmentsSyncTimeAttribute); | ||
| if (it == item.end()) { | ||
| return tl::make_unexpected( |
There was a problem hiding this comment.
Other implementations seem more lenient here. Though I wonder if there was an evolution where maybe this field didn't exist originally.

Ticket: SDK-2363 · Follows #534
Summary
Adds the public
IBigSegmentStoreinterface, theMembership/StoreMetadatavalue types, and concrete DynamoDB + Redis implementations. Schema strings match what the Relay Proxy writes.Design notes
synchronizedOnparsing. Stored as DynamoDB N / Redis string. Both stores reject malformed values (non-numeric strings, wrong DynamoDB attribute type) rather than silently returning 0, matching the existingdynamodb_source.cpprow validation.Not in scope
BigSegmentsBuilderconfig plumbing.BigSegmentStoreWrapper(LRU cache + staleness polling) and the hashing path.rules.cppbig-segments TODO.Test plan
Membershipunit tests pass.RedisBigSegmentStoreintegration tests pass against Redis 7 (docker).DynamoDBBigSegmentStoreintegration tests pass against DynamoDB Local.Note
Medium Risk
New evaluation-adjacent persistence APIs and external-store reads; behavior is well-tested but not yet on the hot path until wrapper/evaluator land.
Overview
Introduces Big Segments read-only persistence for the C++ server SDK: a public
IBigSegmentStorecontract plusMembership/StoreMetadatavalue types, with Redis and DynamoDB stores that match Relay Proxy key/layout conventions (prefix-scoped, shared tables/DBs with existing data sources).Stores implement opaque context-hash membership lookups (include/exclude segment refs, inclusion wins on overlap) and staleness metadata (
synchronizedOn), with strict validation on malformed DynamoDB attribute types and non-numeric sync timestamps instead of silent empty/zero results. CMake and integration tests coverMembershipsemantics, Redis/DynamoDB happy paths, prefix isolation, and error paths.Not in this PR: config builder wiring, caching/staleness wrapper, context hashing, or flag evaluation integration.
Reviewed by Cursor Bugbot for commit b99bf0c. Bugbot is set up for automated code reviews on this repo. Configure here.