Skip to content

feat: add IBigSegmentStore interface + Redis and DynamoDB stores#536

Open
beekld wants to merge 6 commits into
mainfrom
beeklimt/SDK-2363
Open

feat: add IBigSegmentStore interface + Redis and DynamoDB stores#536
beekld wants to merge 6 commits into
mainfrom
beeklimt/SDK-2363

Conversation

@beekld
Copy link
Copy Markdown
Contributor

@beekld beekld commented May 19, 2026

Ticket: SDK-2363 · Follows #534

Summary

Adds the public IBigSegmentStore interface, the Membership / StoreMetadata value types, and concrete DynamoDB + Redis implementations. Schema strings match what the Relay Proxy writes.

auto redis_store = RedisBigSegmentStore::Create("redis://localhost:6379", "prefix");
auto dynamo_store = DynamoDBBigSegmentStore::Create("my-table", "prefix", options);

Design notes

  • synchronizedOn parsing. 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 existing dynamodb_source.cpp row validation.
  • No hashing here. The interface contract says callers pass the already-hashed base64 SHA-256 context key; the SDK will hash in the wrapper, not the store implementations.

Not in scope

  • BigSegmentsBuilder config plumbing.
  • BigSegmentStoreWrapper (LRU cache + staleness polling) and the hashing path.
  • Evaluator wiring / replacing the rules.cpp big-segments TODO.

Test plan

  • 7 Membership unit tests pass.
  • 9 RedisBigSegmentStore integration tests pass against Redis 7 (docker).
  • 8 DynamoDBBigSegmentStore integration tests pass against DynamoDB Local.
  • Full server-sdk test suite (468 tests) still green.
  • Schema constants match what Relay writes (verified against the LocalStack/Redis fixture data).

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 IBigSegmentStore contract plus Membership / StoreMetadata value 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 cover Membership semantics, 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.

@beekld beekld marked this pull request as ready for review May 20, 2026 18:50
@beekld beekld requested a review from a team as a code owner May 20, 2026 18:50
Comment thread libs/server-sdk-dynamodb-source/src/dynamodb_big_segment_store.cpp
Comment thread libs/server-sdk-dynamodb-source/src/dynamodb_big_segment_store.cpp
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread libs/server-sdk-redis-source/src/redis_big_segment_store.cpp
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) {
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.

Should we catch std::exception as well?

public:
DynamoDBBigSegmentTests()
: table_name_("ld-dynamodb-big-segments-test"),
prefix_("testprefix"),
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.

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());
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.

Do we need to sanitize this for the redis user/password?

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.

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;
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.

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) {}
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.

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(
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.

Other implementations seem more lenient here. Though I wonder if there was an evolution where maybe this field didn't exist originally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants