Skip to content

add BLPOP debug log#148

Open
MrGuin wants to merge 1 commit intoeloqdata:mainfrom
MrGuin:add_blpop_debug_log
Open

add BLPOP debug log#148
MrGuin wants to merge 1 commit intoeloqdata:mainfrom
MrGuin:add_blpop_debug_log

Conversation

@MrGuin
Copy link
Collaborator

@MrGuin MrGuin commented Sep 9, 2025

For #109 .

Summary by CodeRabbit

  • Chores
    • Enhanced debug logging around blocking-list pop expiration timing, now emitting timestamps when an item is considered expired.
    • Added constructor-time logging of expiration values to aid troubleshooting.
    • Introduced conditional debug logs for specific list-push scenarios to support targeted diagnostics.
    • Added diagnostic logs during list append operations when inserting certain values.
    • No functional or interface changes; behavior unchanged.

@MrGuin MrGuin mentioned this pull request Sep 9, 2025
@MrGuin MrGuin force-pushed the add_blpop_debug_log branch 2 times, most recently from d499157 to 260a498 Compare September 10, 2025 09:58
@MrGuin MrGuin force-pushed the add_blpop_debug_log branch 2 times, most recently from 91d75d3 to b753a3f Compare September 15, 2025 10:10
@MrGuin MrGuin force-pushed the add_blpop_debug_log branch from b753a3f to d9a1a5d Compare September 19, 2025 05:39
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Adds conditional debug/info logging across BLMPop expiration checks, BLMPop constructor, RPUSH parsing for specific test keys, and list append path for a specific element. No control-flow, API, data structure, or behavior changes beyond emitting logs.

Changes

Cohort / File(s) Summary of Changes
BLMPop expiration logging
include/redis_command.h
Cache ClockTs() into a local variable, compare to ts_expired_, and emit an info log when expired; no functional change to return value.
Constructor and RPUSH parse logging
src/redis_command.cpp
Add debug logs: constructor logs ts_expired; ParseRPushCommand logs when args match specific test values; logic unchanged.
List commit diagnostic logging
src/redis_list_object.cpp
Add diagnostic log when appending single "foo" element to an empty list in CommitRPush; no logic or state changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my ears at logs that pop,
A timestamp hop, a gentle stop.
In lists I stash a secret “foo,”
And note it with a timid coo.
With traces bright, I bound and see—
Debugging trails for you and me. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately summarizes the primary change of adding a BLPOP debug log, is concise and focused, and reflects the main intent of the PR without unnecessary detail or noise.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
include/redis_command.h (1)

2876-2883: Compute predicate once; ensure logging macro is available in TU

Tiny nit: avoid re-evaluating clock_ts > ts_expired_; compute once and return the bool. Also, since this is in a header, verify all TUs including this header have DLOG defined (or include butil/logging here, or move this method to a .cpp) to prevent build fragility.

-        auto clock_ts = txservice::LocalCcShards::ClockTs();
-        if (clock_ts > ts_expired_)
-        {
-            DLOG(INFO) << "BLMPopCommand, expired, ClockTs: " << clock_ts
-                       << ", ts_expired: " << ts_expired_;
-        }
-        return clock_ts > ts_expired_;
+        const auto clock_ts = txservice::LocalCcShards::ClockTs();
+        const bool expired = clock_ts > ts_expired_;
+        if (expired) {
+            DLOG(INFO) << "BLMPopCommand expired: now=" << clock_ts
+                       << " deadline=" << ts_expired_;
+        }
+        return expired;

If DLOG isn’t guaranteed in all includers, either add #include "butil/logging.h" here or move this method out-of-line to a .cpp.

src/redis_list_object.cpp (1)

644-649: Gate the test-specific “foo” log behind a flag or remove before ship

Hard-coding a sentinel value in core write path is brittle. Keep it if you need it for #109, but please guard with a build/runtime flag (e.g., #ifdef ELOQKV_DEBUG_BLPOP) or plan to remove post‑debug to avoid surprising noise later.

src/redis_command.cpp (1)

11522-11525: Gate test-only key/value logging to avoid leaking user data in prod.

This emits keys and values for specific test inputs. Guard behind a build flag to keep production logs clean.

Apply this minimal guard:

-    if ((args[1] == "blist1{t}" || args[1] == "blist2{t}") && args[2] == "foo")
-    {
-        DLOG(INFO) << "rpush : " << args[1] << " " << args[2];
-    }
+    #ifdef ELOQ_TEST_LOGS
+    if ((args[1] == "blist1{t}" || args[1] == "blist2{t}") && args[2] == "foo") {
+        DLOG(INFO) << "rpush : " << args[1] << " " << args[2];
+    }
+    #endif

Also, please confirm whether butil’s DLOG is compiled out in release builds in your setup.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 218d665 and d9a1a5d.

📒 Files selected for processing (3)
  • include/redis_command.h (1 hunks)
  • src/redis_command.cpp (2 hunks)
  • src/redis_list_object.cpp (1 hunks)
🔇 Additional comments (1)
src/redis_command.cpp (1)

4980-4981: BLMPop ctor debug log is fine.

Non-invasive, helpful for tracing timeouts. Consider adding VLOG level if available, but OK as-is.

@MrGuin MrGuin force-pushed the add_blpop_debug_log branch from d9a1a5d to 0f06fa5 Compare September 25, 2025 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants