feat: support ignoring Redis TTL for diagnostics - #555
Closed
thweetkomputer wants to merge 1 commit into
Closed
Conversation
Contributor
WalkthroughThe PR adds a diagnostic mode that ignores Redis TTL enforcement at runtime while preserving stored TTL metadata. It updates object behavior, service initialization, configuration documentation, and serialization tests. It also updates the ChangesRedis TTL Ignore Mode
Data Substrate Update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
thweetkomputer
marked this pull request as ready for review
August 7, 2026 02:42
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@data_substrate`:
- Line 1: Update the data_substrate submodule pointer from unreachable commit
99604109c00967d33c07cc6adf167df51bd33331 to a reachable merged commit from
eloqdata/tx_service#546, or refresh it with git submodule update --remote
data_substrate, then commit the updated pointer.
In `@include/redis_hash_object.h`:
- Around line 268-270: Update HasTTL() and GetTTL() in Redis hash, list, set,
string, and zset object classes to report stored TTL metadata rather than the
process-local ignore flag; use the ignore-only flag exclusively in expiry
enforcement and expiry/read/report paths. Apply the corresponding behavior
consistently in include/redis_hash_object.h (268-270),
include/redis_list_object.h (264-266), include/redis_set_object.h (132-134),
include/redis_string_object.h (283-285), and include/redis_zset_object.h (378).
Update eloqkv.ini (101-107), docs/03-data-model.md (88), and
tests/unit/eloq/object_serialize_deserialize_test.cpp (39-47) only as needed to
reflect and verify the preserved TTL metadata semantics.
In `@include/redis_object.h`:
- Around line 66-80: Update the documentation for the public
RedisEloqObject::SetIgnoreTTL() and IgnoreTTL() APIs to state that the
process-wide mode is configured once during initialization and must not change
while commands execute. Also document that the atomic acquire/release operations
provide visibility only; they do not make runtime mode changes semantically
consistent.
In `@src/redis_service.cpp`:
- Around line 244-251: Update RedisServiceImpl::Init() to parse the
store.ignore_redis_ttl configuration and --ignore_redis_ttl command-line option
before calling EloqDS::IgnoreRedisTTL() and RedisEloqObject::SetIgnoreTTL().
Ensure the parsed setting is applied consistently before the existing warning
logic.
In `@tests/unit/eloq/object_serialize_deserialize_test.cpp`:
- Around line 32-51: The test case “ignore Redis TTL preserves serialized
metadata” must restore the process-wide SetIgnoreTTL state even when an
assertion or exception interrupts execution. Capture the prior ignore-TTL value
before modifying it, then use an RAII guard scoped to the test to restore that
value automatically; retain the existing assertions and serialization behavior.
🪄 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: 1f6adfa2-4b85-4264-8ce9-f0c71a3efb21
📒 Files selected for processing (11)
data_substratedocs/03-data-model.mdeloqkv.iniinclude/redis_hash_object.hinclude/redis_list_object.hinclude/redis_object.hinclude/redis_set_object.hinclude/redis_string_object.hinclude/redis_zset_object.hsrc/redis_service.cpptests/unit/eloq/object_serialize_deserialize_test.cpp
| @@ -1 +1 @@ | |||
| Subproject commit 63db0ecbe3cd04e23282aa61af3961491647bddf | |||
| Subproject commit 99604109c00967d33c07cc6adf167df51bd33331 | |||
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
expected='99604109c00967d33c07cc6adf167df51bd33331'
test "$(git -C data_substrate rev-parse HEAD)" = "$expected"
git -C data_substrate cat-file -e "${expected}^{commit}"
pr_json="$(gh api repos/eloqdata/tx_service/pulls/546)"
test "$(jq -r '.state' <<<"$pr_json")" = "closed"
test "$(jq -r '.merged_at' <<<"$pr_json")" != "null"
test "$(jq -r '.merge_commit_sha' <<<"$pr_json")" = "$expected"Repository: eloqdata/eloqkv
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CLAUDE.md presence:"
[ -f CLAUDE.md ] && echo "yes" || echo "no"
[ -f CL AUDE.md ] && echo "CL AUDE.md exists" || true
echo
echo "Submodule files:"
git ls-files data_substrate | head -80 || true
echo
echo "Gitlink in index:"
git ls-files -s data_substrate | sed -n '1p' || true
echo
echo "data_substrate submodule status:"
git submodule status data_substrate || true
echo
echo "CLAUDE.md size/head if present:"
[ -f CLAUDE.md ] && wc -l CLAUDE.md && sed -n '1,220p' CLAUDE.md
echo
echo "Relevant files containing data_substrate:"
rg -n "data_substrate|tx_service|ttl" -S --glob '!**/.git/**' 2>/dev/null | sed -n '1,220p'Repository: eloqdata/eloqkv
Length of output: 6992
🏁 Script executed:
#!/bin/bash
set -u
expected='99604109c00967d33c07cc6adf167df51bd33331'
echo "Submodule tracked path:"
git ls-files --stage data_substrate data_substrate/ 2>/dev/null || true
echo
echo "Check if expected SHA is reachable from tracked commit (submodule worktree may be absent):"
if git -C data_substrate rev-parse --verify "$expected^{commit}" >/dev/null 2>&1; then
echo "expected_sha: $expected"
echo "expected_ref: $(git -C data_substrate rev-parse HEAD)"
git -C data_substrate merge-base --is-ancestor "$expected" HEAD; echo "ancestor_exit=$?"
git -C data_substrate merge-base --is-ancestor HEAD "$expected"; echo "reverse_ancestor_exit=$?"
else
echo "expected SHA not found in submodule worktree"
fi
echo
echo "Upstream PR `#546`:"
pr_json="$(gh api repos/eloqdata/tx_service/pulls/546 --silent)"
printf '%s\n' "$pr_json" | jq '{number,state,title,merged,merged_at,merge_commit_sha}' 2>/dev/null || { printf '%s\n' "$pr_json"; exit 1; }
echo
echo "Remote heads/tags for tx_service relevant refs (no checkout/build):"
git ls-remote --heads --tags https://github.com/eloqdata/tx_service.git \
refs/heads/main refs/heads/master refs/heads/develop "$expected" 2>/dev/null | sed -n '1,200p' || true
echo
echo "Parent submodule URL from .gitmodules/data_substrate:"
git config --file .gitmodules submodule.data_substrate.url || trueRepository: eloqdata/eloqkv
Length of output: 622
🏁 Script executed:
#!/bin/bash
set -u
expected='99604109c00967d33c07cc6adf167df51bd33331'
echo "git ls-remote all matching $expected:"
git ls-remote https://github.com/eloqdata/tx_service.git "$expected" 2>/dev/null || true
echo
echo "Fetch lightweight tag object if exists:"
tmpdir="$(mktemp -d)"
export GIT_DIR="$tmpdir/empty.git"
git -C "$tmpdir" init --bare >/dev/null
git ls-remote --tags https://github.com/eloqdata/tx_service.git "$expected" 2>/dev/null || true
echo
echo "Resolve SHA by direct GitHub API ref lookup (tags and heads):"
for ref in refs/tags "$expected" refs/heads/main refs/heads/master refs/heads/develop; do
echo "--- $ref"
gh api "repos/eloqdata/tx_service/git/ref/$ref" --silent 2>/dev/null | jq '{ref,object:{sha,type}}' || true
done
echo
echo "Compare current PR branch heads:"
for name in main master develop; do
sha="$(git ls-remote https://github.com/eloqdata/tx_service.git "refs/heads/$name" 2>/dev/null | awk '{print $1}')"
echo "$name: $sha"
done
echo "expected: $expected"Repository: eloqdata/eloqkv
Length of output: 1169
Fix the data_substrate submodule pointer.
99604109c00967d33c07cc6adf167df51bd33331 does not exist in eloqdata/tx_service, so this will break git submodule update --init --recursive. Use a reachable merged commit from eloqdata/tx_service#546 or re-run git submodule update --remote data_substrate and commit that pointer.
🤖 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 `@data_substrate` at line 1, Update the data_substrate submodule pointer from
unreachable commit 99604109c00967d33c07cc6adf167df51bd33331 to a reachable
merged commit from eloqdata/tx_service#546, or refresh it with git submodule
update --remote data_substrate, then commit the updated pointer.
Comment on lines
268
to
+270
| bool HasTTL() const override | ||
| { | ||
| return true; | ||
| return !IgnoreTTL(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "data_substrate revision:"
git -C data_substrate rev-parse HEAD
echo "data_substrate TTL-ignore and storage paths:"
git -C data_substrate grep -n -C 8 -E \
'IgnoreRedisTTL|ignore_redis_ttl|TTLCompactionFilter|BatchWriteRecords|HasTTL|GetTTL' \
-- . || true
echo "Redis HasTTL() consumers:"
git grep -n -C 5 -E 'HasTTL\(\)|GetTTL\(\)' \
-- include src tests || trueRepository: eloqdata/eloqkv
Length of output: 21507
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Object method declarations and implementations:"
for f in include/redis_hash_object.h include/redis_list_object.h include/redis_set_object.h include/redis_string_object.h include/redis_zset_object.h src/redis_eloq_object.cpp; do
if [ -f "$f" ]; then
echo "== $f =="
rg -n -C 4 'class Redis(Hash|List|Set|String|Zset)(TTL)?Object|AddTTL|RemoveTTL|Serialize(.*const|\(\))\s*\{|void SetTTL|bool HasTTL|uint64_t GetTTL|ttl_' "$f" || true
else
echo "missing $f"
fi
done
echo "Redis TTL command and service relevant snippets:"
sed -n '1360,1420p' src/redis_command.cpp
sed -n '7600,7790p' src/redis_command.cpp
sed -n '7800,8280p' src/redis_command.cpp
sed -n '8085,8135p' src/redis_command.cpp
sed -n '5005,5035p' src/redis_service.cpp
echo "Repository-wide IgnoreTTL usages:"
rg -n -C 4 'SetIgnoreTTL|IgnoreTTL|ignore_redis_ttl' . --glob '!data_substrate' || trueRepository: eloqdata/eloqkv
Length of output: 47193
Preserve TTL semantics while bypassing expiry enforcement.
HasTTL() now reads a process-local flag, but EXPIRE/PERSIST/TTL commands and checkpoint/TTL-compaction paths use HasTTL() as a metadata presence check. Keep HasTTL()/GetTTL() reflecting the stored TTL, and use the ignore-only flag where expiry enforcement/read/report behavior should be bypassed.
📍 Affects 8 files
include/redis_hash_object.h#L268-L270(this comment)include/redis_list_object.h#L264-L266include/redis_set_object.h#L132-L134include/redis_string_object.h#L283-L285include/redis_zset_object.h#L378-L378eloqkv.ini#L101-L107docs/03-data-model.md#L88-L88tests/unit/eloq/object_serialize_deserialize_test.cpp#L39-L47
🤖 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 `@include/redis_hash_object.h` around lines 268 - 270, Update HasTTL() and
GetTTL() in Redis hash, list, set, string, and zset object classes to report
stored TTL metadata rather than the process-local ignore flag; use the
ignore-only flag exclusively in expiry enforcement and expiry/read/report paths.
Apply the corresponding behavior consistently in include/redis_hash_object.h
(268-270), include/redis_list_object.h (264-266), include/redis_set_object.h
(132-134), include/redis_string_object.h (283-285), and
include/redis_zset_object.h (378). Update eloqkv.ini (101-107),
docs/03-data-model.md (88), and
tests/unit/eloq/object_serialize_deserialize_test.cpp (39-47) only as needed to
reflect and verify the preserved TTL metadata semantics.
Comment on lines
+66
to
+80
| /** | ||
| * Controls the diagnostic mode that exposes persisted Redis objects even | ||
| * after their expiration timestamp. The mode changes only runtime TTL | ||
| * interpretation; serialized TTL metadata remains intact so disabling the | ||
| * mode on a later restart restores normal expiration. | ||
| */ | ||
| static void SetIgnoreTTL(bool ignore_ttl) | ||
| { | ||
| ignore_ttl_.store(ignore_ttl, std::memory_order_release); | ||
| } | ||
|
|
||
| static bool IgnoreTTL() | ||
| { | ||
| return ignore_ttl_.load(std::memory_order_acquire); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the startup-only and atomic-state contract.
RedisEloqObject::IgnoreTTL() is a new public API without a documentation comment. Document that SetIgnoreTTL() sets one process-wide mode during initialization and must not change while commands run. Atomic visibility does not make live mode changes semantically consistent.
As per coding guidelines, **/*.{h,hpp} requires documentation comments for new public APIs, and **/*.{c,cc,cpp,h,hpp} requires non-obvious concurrency and memory-ordering assumptions to be documented.
Proposed documentation
/**
- * Controls the diagnostic mode that exposes persisted Redis objects even
- * after their expiration timestamp. The mode changes only runtime TTL
- * interpretation; serialized TTL metadata remains intact so disabling the
- * mode on a later restart restores normal expiration.
+ * Sets the process-wide diagnostic mode. Call this during initialization
+ * before command workers access Redis objects. Do not change the mode
+ * during normal operation.
*/
static void SetIgnoreTTL(bool ignore_ttl)
+ /**
+ * Returns whether diagnostic TTL-ignore mode is enabled.
+ * Atomic access publishes the setting across worker threads but does not
+ * make runtime mode changes semantically consistent.
+ */
static bool IgnoreTTL()📝 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
| /** | |
| * Controls the diagnostic mode that exposes persisted Redis objects even | |
| * after their expiration timestamp. The mode changes only runtime TTL | |
| * interpretation; serialized TTL metadata remains intact so disabling the | |
| * mode on a later restart restores normal expiration. | |
| */ | |
| static void SetIgnoreTTL(bool ignore_ttl) | |
| { | |
| ignore_ttl_.store(ignore_ttl, std::memory_order_release); | |
| } | |
| static bool IgnoreTTL() | |
| { | |
| return ignore_ttl_.load(std::memory_order_acquire); | |
| } | |
| /** | |
| * Sets the process-wide diagnostic mode. Call this during initialization | |
| * before command workers access Redis objects. Do not change the mode | |
| * during normal operation. | |
| */ | |
| static void SetIgnoreTTL(bool ignore_ttl) | |
| { | |
| ignore_ttl_.store(ignore_ttl, std::memory_order_release); | |
| } | |
| /** | |
| * Returns whether diagnostic TTL-ignore mode is enabled. | |
| * Atomic access publishes the setting across worker threads but does not | |
| * make runtime mode changes semantically consistent. | |
| */ | |
| static bool IgnoreTTL() | |
| { | |
| return ignore_ttl_.load(std::memory_order_acquire); | |
| } |
🤖 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 `@include/redis_object.h` around lines 66 - 80, Update the documentation for
the public RedisEloqObject::SetIgnoreTTL() and IgnoreTTL() APIs to state that
the process-wide mode is configured once during initialization and must not
change while commands execute. Also document that the atomic acquire/release
operations provide visibility only; they do not make runtime mode changes
semantically consistent.
Source: Coding guidelines
Comment on lines
+244
to
+251
| const bool ignore_redis_ttl = EloqDS::IgnoreRedisTTL(); | ||
| RedisEloqObject::SetIgnoreTTL(ignore_redis_ttl); | ||
| if (ignore_redis_ttl) | ||
| { | ||
| LOG(WARNING) << "ignore_redis_ttl is enabled: persisted expiration " | ||
| "timestamps will be retained but not enforced"; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
git -C data_substrate grep -n -C 8 -E \
'IgnoreRedisTTL|ignore_redis_ttl|Initialize' -- . || true
git grep -n -C 8 -E \
'RedisServiceImpl::Init|DataSubstrate::Instance|config_file_|ignore_redis_ttl' \
-- src include eloqkv.ini || trueRepository: eloqdata/eloqkv
Length of output: 24657
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate ignore_redis_ttl =="
rg -n -C 6 'ignore_redis_ttl|IgnoreRedisTTL|SetIgnoreRedisTTL|GetIgnoreRedisTTL' -- . || true
echo "== locate data_substrate init references =="
rg -n -C 5 'DataSubstrate.*Init|Init\(.*config|config.*ignore_redis_ttl|ParseError|CommandLineOption' src include --glob '*.{cpp,cc,c,hpp,h,hxx,cc}' || true
echo "== outline ignore_redis_ttl header/source =="
fd -a 'ignore_redis_ttl\.h|ignore_redis_ttl\.cpp' . || true
for f in $(fd 'ignore_redis_ttl\.(h|cpp)$' .); do echo "--- $f"; ast-grep outline "$f" || true; sed -n '1,240p' "$f"; done
echo "== redis_server startup =="
sed -n '1,120p' src/redis_server.cpp
sed -n '420,475p' src/redis_server.cpp
echo "== redis_service init section =="
sed -n '226,255p' src/redis_service.cppRepository: eloqdata/eloqkv
Length of output: 36885
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== data_substrate header/source candidates =="
git ls-files | rg '(^|/)(data_substrate\.(h|cpp|hpp|cc)$|data_substrate/.*)|(eloq_data_store_service/ignore_redis_ttl\.(h|cpp)$)' || true
echo "== exact matches in git tracked files =="
git ls-files | xargs -r grep -n -H -S -C 4 'IgnoreRedisTTL|ignore_redis_ttl|SetIgnoreRedisTTL|GetIgnoreRedisTTL' || true
echo "== precise function/header definitions =="
rg -n -C 8 'bool\s+IgnoreRedisTTL|void\s+SetIgnoreRedisTTL|SET_IGNORE|IGNORE_REDIS|ignore_redis_ttl' . || true
echo "== startup flag parsing candidates =="
rg -n -C 4 'DEFINE_bool.*ignore|GetCommandLineOption|ignore_redis_ttl|ParseCommandLineFlags|SetCommandLineOption' src include --glob '*.{cpp,cc,h,hpp,cc}' || trueRepository: eloqdata/eloqkv
Length of output: 29601
Parse ignore_redis_ttl before applying it.
RedisServiceImpl::Init() applies EloqDS::IgnoreRedisTTL() without reading store.ignore_redis_ttl from config_. If this setting is advertised as configurable, map store.ignore_redis_ttl and --ignore_redis_ttl through RedisServiceImpl::Init() before RedisEloqObject::SetIgnoreTTL().
🤖 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 `@src/redis_service.cpp` around lines 244 - 251, Update
RedisServiceImpl::Init() to parse the store.ignore_redis_ttl configuration and
--ignore_redis_ttl command-line option before calling EloqDS::IgnoreRedisTTL()
and RedisEloqObject::SetIgnoreTTL(). Ensure the parsed setting is applied
consistently before the existing warning logic.
Comment on lines
+32
to
+51
| TEST_CASE("ignore Redis TTL preserves serialized metadata") | ||
| { | ||
| EloqKV::RedisStringTTLObject object; | ||
| object.SetTTL(12345); | ||
|
|
||
| EloqKV::RedisEloqObject::SetIgnoreTTL(false); | ||
| REQUIRE(object.HasTTL()); | ||
| std::string before; | ||
| object.Serialize(before); | ||
|
|
||
| EloqKV::RedisEloqObject::SetIgnoreTTL(true); | ||
| REQUIRE_FALSE(object.HasTTL()); | ||
| REQUIRE(object.GetTTL() == 12345); | ||
| std::string ignored; | ||
| object.Serialize(ignored); | ||
| REQUIRE(ignored == before); | ||
|
|
||
| EloqKV::RedisEloqObject::SetIgnoreTTL(false); | ||
| REQUIRE(object.HasTTL()); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test file and relevant implementation symbols.
printf '--- files ---\n'
git ls-files | rg '(^|/)object_serialize_deserialize_test\.cpp$|RedisEloqObject|RedisStringTTLObject|IgnoreTTL|SetIgnoreTTL' || true
printf '\n--- test file excerpt ---\n'
sd='tests/unit/eloq/object_serialize_deserialize_test.cpp'
if [ -f "$sd" ]; then
nl -ba "$sd" | sed -n '1,120p'
fi
printf '\n--- symbol search ---\n'
rg -n "SetIgnoreTTL|IgnoreTTL|class RedisEloqObject|RedisStringTTLObject" . --glob '!build/**' --glob '!dist/**' | head -200Repository: eloqdata/eloqkv
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test file and relevant implementation symbols.
echo '--- files ---'
git ls-files | rg '(^|/)object_serialize_deserialize_test\.cpp$|RedisEloqObject|RedisStringTTLObject|IgnoreTTL|SetIgnoreTTL' || true
echo
echo '--- test file excerpt ---'
sd='tests/unit/eloq/object_serialize_deserialize_test.cpp'
if [ -f "$sd" ]; then
nl -ba "$sd" | sed -n '1,120p'
fi
echo
echo '--- symbol search ---'
rg -n "SetIgnoreTTL|IgnoreTTL|class RedisEloqObject|RedisStringTTLObject" . --glob '!build/**' --glob '!dist/**' | head -200Repository: eloqdata/eloqkv
Length of output: 289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test file excerpt ---'
sed -n '1,120p' tests/unit/eloq/object_serialize_deserialize_test.cpp | \
nl -ba
echo
echo '--- symbol search ---'
rg -n "SetIgnoreTTL|IgnoreTTL|class RedisEloqObject|RedisStringTTLObject" . --glob '!build/**' --glob '!dist/**' | head -200
echo
echo '--- definitions/usages ---'
rg -n "SetIgnoreTTL|Ignor(e|Ing)TTL|RedisStringTTLObject::|SetTTL|HasTTL|GetTTL|Serialize\\(" . --glob '!build/**' --glob '!dist/**' | head -300Repository: eloqdata/eloqkv
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test file excerpt ---'
awk '{printf "%6d\t%s\n", NR, $0}' tests/unit/eloq/object_serialize_deserialize_test.cpp | sed -n '1,160p'
echo
echo '--- symbol search ---'
rg -n "SetIgnoreTTL|IgnoreTTL|class RedisEloqObject|RedisStringTTLObject" . --glob '!build/**' --glob '!dist/**' | head -200
echo
echo '--- TTL serialization definitions/usages ---'
rg -n "SetIgnoreTTL|Ignor(e|Ing)TTL|RedisStringTTLObject::|RedisEloqObject::|SetTTL|HasTTL|GetTTL|Serialize\\(" . --glob '!build/**' --glob '!dist/**' | head -400Repository: eloqdata/eloqkv
Length of output: 42758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- redis_object.h relevant section ---'
sed -n '1,130p' include/redis_object.h
echo
echo '--- redis_service.cpp ignore_redis_ttl setup ---'
sed -n '220,255p' src/redis_service.cpp
echo
echo '--- search for SetIgnoreTTL calls ---'
rg -n "SetIgnoreTTL\\(" . --glob '!build/**' --glob '!dist/**'Repository: eloqdata/eloqkv
Length of output: 5390
Restore RedisEloqObject::SetIgnoreTTL after this test.
This test writes a process-wide atomic flag after the Catch2 @section setup. A fatal assertion or exception before the final reset can leave later tests with ignored Redis TTLs; save the previous value and restore it from an RAII guard.
🤖 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 `@tests/unit/eloq/object_serialize_deserialize_test.cpp` around lines 32 - 51,
The test case “ignore Redis TTL preserves serialized metadata” must restore the
process-wide SetIgnoreTTL state even when an assertion or exception interrupts
execution. Capture the prior ignore-TTL value before modifying it, then use an
RAII guard scoped to the test to restore that value automatically; retain the
existing assertions and serialization behavior.
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
Depends on eloqdata/tx_service#546.
Customers diagnosing large Redis keys need to inspect records that remain in RocksDB after their TTL has expired. The diagnostic cluster must expose those records without destroying the stored TTL, so normal expiration can be restored afterward.
Behavior before and after
By default, behavior is unchanged. With
--ignore_redis_ttl=trueor[store] ignore_redis_ttl=truein a RocksDB-backed deployment, persisted TTL objects remain visible and report no active TTL while the mode is enabled. Disabling the option and restarting restores expiration from the original stored absolute timestamp.Implementation
HasTTL() == falseonly in diagnostic mode while preserving their embedded TTL and serialized bytes.eloqkv.iniand the data-model guide.data_substrateto the implementation in feat: add diagnostic Redis TTL bypass tx_service#546.Design decisions and alternatives
The mode changes runtime interpretation instead of clearing the DSS version-ts MSB or editing Redis object blobs. That makes it reversible and avoids an export/import rewrite. The setting is intentionally static after service initialization; changing it requires a restart so readers, checkpoints, scans, and compaction use one consistent policy.
Test plan
Commands and results:
Risk assessment
The default is
false, so existing deployments are unaffected. When enabled, expired records become visible and ordinary writes still persist; this mode must therefore be used only on an isolated diagnostic store. Every EloqKV and DSS process accessing that store must use the same setting. Once disabled, already-expired records disappear again and become eligible for compaction.The current submodule pointer targets the review commit from eloqdata/tx_service#546 and must be updated to that PR's merged commit before this PR is merged.
Rollback plan
Set
ignore_redis_ttl=falseeverywhere and restart; no data migration is required because serialized TTL metadata is unchanged. The commits can also be reverted.Reviewer guide
Review
RedisEloqObject::SetIgnoreTTL/IgnoreTTLand the TTL subclasses'HasTTL()overrides first, thenRedisServiceImpl::Init. The main invariant is that ignoring TTL affects runtime checks only and never changes serialization.Follow-up work
After eloqdata/tx_service#546 is squash-merged, update this PR's submodule pointer to the merged
maincommit and rerun the RocksDB Cloud build and runtime diagnostic scenario.Summary by CodeRabbit
New Features
--ignore_redis_ttl=trueor[store] ignore_redis_ttl=true.Tests