feat: add diagnostic Redis TTL bypass - #546
Open
thweetkomputer wants to merge 1 commit into
Open
Conversation
WalkthroughAdds the ChangesRedis TTL bypass
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FetchRecordCallback
participant DataStoreServiceScanner
participant TTLCompactionFilter
participant IgnoreRedisTTL
Client->>FetchRecordCallback: Fetch record
FetchRecordCallback->>IgnoreRedisTTL: Check bypass flag
IgnoreRedisTTL-->>FetchRecordCallback: Return flag state
FetchRecordCallback-->>Client: Filter or return expired record
Client->>DataStoreServiceScanner: Scan records
DataStoreServiceScanner->>IgnoreRedisTTL: Check bypass flag
IgnoreRedisTTL-->>DataStoreServiceScanner: Return flag state
DataStoreServiceScanner-->>Client: Filter or return expired records
TTLCompactionFilter->>IgnoreRedisTTL: Check bypass flag
IgnoreRedisTTL-->>TTLCompactionFilter: Return flag state
TTLCompactionFilter-->>TTLCompactionFilter: Delete or preserve expired records
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/09-store-handler.md`:
- Line 127: Update the “Diagnostic TTL bypass (EloqDSS RocksDB variants)”
documentation to state that an explicitly supplied --ignore_redis_ttl
command-line value takes precedence over [store] ignore_redis_ttl, including
that --ignore_redis_ttl=false overrides an INI value of true. Briefly explain
the operational constraint that all participating processes must use the same
effective setting.
In `@tx_service/tests/TTLCompactionFilter-Test.cpp`:
- Around line 59-70: Update both TTL-related sections in the test around
ShouldFilter to restore FLAGS_ignore_redis_ttl via an RAII scoped guard that
captures its prior value and restores it on scope exit, including assertion
failures. Remove the manual reset and preserve each section’s intended flag
setting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3388fd85-906a-4f4c-a5e9-8aa853ad14ff
📒 Files selected for processing (7)
docs/09-store-handler.mdstore_handler/data_store_service_client_closure.cppstore_handler/data_store_service_scanner.cppstore_handler/eloq_data_store_service/ignore_redis_ttl.hstore_handler/eloq_data_store_service/rocksdb_config.cppstore_handler/eloq_data_store_service/rocksdb_data_store_common.cpptx_service/tests/TTLCompactionFilter-Test.cpp
| ## 6. TTL and Purge Mechanisms | ||
|
|
||
| - **Record TTL (data plane).** `BatchWriteRecords` items carry a `ttl` (ms epoch). On read, `FetchRecordCallback` treats an expired EloqKV record as `RecordStatus::Deleted` even if the store still has it. Physical reclamation is compaction-driven: `TTLCompactionFilter` (both `rocksdb_handler.h` and DSS `rocksdb_data_store_common.h`) drops expired entries during RocksDB compaction; the DSS variant flags TTL presence in the version-ts MSB (`MSB`/`MSB_MASK`). | ||
| - **Diagnostic TTL bypass (EloqDSS RocksDB variants).** `--ignore_redis_ttl=true` (or `[store] ignore_redis_ttl=true`) bypasses the EloqKV point-read, bucket, and scanner expiry checks and makes `TTLCompactionFilter` retain expired records. EloqKV separately makes its TTL object variants report no TTL while the mode is active. Neither the version-ts MSB, the outer expiration timestamp, nor the Redis object's embedded TTL is rewritten, so restarting every EloqKV and DSS process with the option disabled restores normal expiration from the stored absolute timestamp. All participating processes must use the same setting; the option is intended for isolated diagnostic stores, and it does not make ordinary writes read-only. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document command-line precedence.
An explicit --ignore_redis_ttl value overrides [store] ignore_redis_ttl. State this rule, including that --ignore_redis_ttl=false overrides an INI value of true.
As per coding guidelines, document non-obvious operational constraints and explain why.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/09-store-handler.md` at line 127, Update the “Diagnostic TTL bypass
(EloqDSS RocksDB variants)” documentation to state that an explicitly supplied
--ignore_redis_ttl command-line value takes precedence over [store]
ignore_redis_ttl, including that --ignore_redis_ttl=false overrides an INI value
of true. Briefly explain the operational constraint that all participating
processes must use the same effective setting.
Source: Coding guidelines
Comment on lines
+59
to
+70
| FLAGS_ignore_redis_ttl = false; | ||
| const std::string value = MakeValue(EloqDS::MSB | 42, 1); | ||
| REQUIRE(ShouldFilter(value, kCompactionTimestamp)); | ||
| } | ||
|
|
||
| SECTION("expired TTL value is retained in diagnostic mode") | ||
| { | ||
| FLAGS_ignore_redis_ttl = true; | ||
| const std::string value = MakeValue(EloqDS::MSB | 42, 1); | ||
| REQUIRE_FALSE(ShouldFilter(value, kCompactionTimestamp)); | ||
| FLAGS_ignore_redis_ttl = false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore FLAGS_ignore_redis_ttl with RAII.
If REQUIRE_FALSE fails on Line 68, Catch2 exits the section before Line 69. Later tests then run with TTL bypass enabled. Preserve and restore the prior flag value with a scoped guard in each section.
Proposed test isolation fix
+class ScopedIgnoreRedisTTLFlag
+{
+ public:
+ explicit ScopedIgnoreRedisTTLFlag(bool value)
+ : previous_value_(FLAGS_ignore_redis_ttl)
+ {
+ FLAGS_ignore_redis_ttl = value;
+ }
+
+ ~ScopedIgnoreRedisTTLFlag()
+ {
+ FLAGS_ignore_redis_ttl = previous_value_;
+ }
+
+ private:
+ bool previous_value_;
+};
+
- FLAGS_ignore_redis_ttl = false;
+ ScopedIgnoreRedisTTLFlag ttl_flag(false);
const std::string value = MakeValue(EloqDS::MSB | 42, 1);
REQUIRE(ShouldFilter(value, kCompactionTimestamp));
- FLAGS_ignore_redis_ttl = true;
+ ScopedIgnoreRedisTTLFlag ttl_flag(true);
const std::string value = MakeValue(EloqDS::MSB | 42, 1);
REQUIRE_FALSE(ShouldFilter(value, kCompactionTimestamp));
- FLAGS_ignore_redis_ttl = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FLAGS_ignore_redis_ttl = false; | |
| const std::string value = MakeValue(EloqDS::MSB | 42, 1); | |
| REQUIRE(ShouldFilter(value, kCompactionTimestamp)); | |
| } | |
| SECTION("expired TTL value is retained in diagnostic mode") | |
| { | |
| FLAGS_ignore_redis_ttl = true; | |
| const std::string value = MakeValue(EloqDS::MSB | 42, 1); | |
| REQUIRE_FALSE(ShouldFilter(value, kCompactionTimestamp)); | |
| FLAGS_ignore_redis_ttl = false; | |
| } | |
| class ScopedIgnoreRedisTTLFlag | |
| { | |
| public: | |
| explicit ScopedIgnoreRedisTTLFlag(bool value) | |
| : previous_value_(FLAGS_ignore_redis_ttl) | |
| { | |
| FLAGS_ignore_redis_ttl = value; | |
| } | |
| ~ScopedIgnoreRedisTTLFlag() | |
| { | |
| FLAGS_ignore_redis_ttl = previous_value_; | |
| } | |
| private: | |
| bool previous_value_; | |
| }; | |
| ScopedIgnoreRedisTTLFlag ttl_flag(false); | |
| const std::string value = MakeValue(EloqDS::MSB | 42, 1); | |
| REQUIRE(ShouldFilter(value, kCompactionTimestamp)); | |
| } | |
| SECTION("expired TTL value is retained in diagnostic mode") | |
| { | |
| ScopedIgnoreRedisTTLFlag ttl_flag(true); | |
| const std::string value = MakeValue(EloqDS::MSB | 42, 1); | |
| REQUIRE_FALSE(ShouldFilter(value, kCompactionTimestamp)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tx_service/tests/TTLCompactionFilter-Test.cpp` around lines 59 - 70, Update
both TTL-related sections in the test around ShouldFilter to restore
FLAGS_ignore_redis_ttl via an RAII scoped guard that captures its prior value
and restores it on scope exit, including assertion failures. Remove the manual
reset and preserve each section’s intended flag setting.
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
1 participant
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.
Context
Customers diagnosing large Redis keys need to inspect records that are still present in RocksDB even after their Redis TTL has expired. The normal point-read, scan, and compaction paths hide or physically reclaim those records.
Behavior before and after
Before this change, EloqDSS treats expired EloqKV records as deleted during reads and scans, and the RocksDB TTL compaction filter eventually removes them.
With
--ignore_redis_ttl=trueor[store] ignore_redis_ttl=true, RocksDB-backed EloqDSS variants expose persisted expired records and retain them during compaction. The default remainsfalse. Stored TTL metadata and value bytes are unchanged, so disabling the option and restarting restores normal expiration from the original absolute timestamp.Implementation
ignore_redis_ttlflag for EloqDSS RocksDB configurations.IgnoreRedisTTL(); non-RocksDB builds always returnfalse.TTLCompactionFilterretain records while diagnostic mode is enabled.Design decisions and alternatives
The implementation bypasses TTL interpretation instead of clearing the version-ts MSB or rewriting serialized values. This preserves the original expiration timestamp and makes the mode reversible without a data migration. Every EloqKV and DSS process accessing the diagnostic store must use the same setting.
Test plan
Commands and results:
Risk assessment
This intentionally changes read visibility and compaction behavior only when explicitly enabled. Inconsistent settings across processes can produce inconsistent visibility, and ordinary writes remain persistent while diagnostic mode is active. Once disabled, records whose absolute TTL is already in the past become expired again.
Rollback plan
Set
ignore_redis_ttl=falseon every EloqKV and DSS process and restart. No stored data conversion is required. The code change can also be reverted independently.Reviewer guide
Start with
ignore_redis_ttl.handrocksdb_config.cpp, then verify the three read/scan gates and the early return inTTLCompactionFilter::Filter. The key invariant is that diagnostic mode never mutates the stored TTL header, MSB, or value bytes.Follow-up work
Run the new unit test binary and an end-to-end RocksDB Cloud diagnostic-cluster scenario in CI.
Summary by CodeRabbit
New Features
ignore_redis_ttldiagnostic option for RocksDB-based configurations.Bug Fixes
Tests