Use "INFO" level to print "key not found" log in SyncBatchReadForArchiveCallback

[lzxddz]

Summary by CodeRabbit

• Bug Fixes
  • Improved robustness of archive data retrieval operations. Missing entries are now handled gracefully by treating them as deleted entries within the batch read process, rather than triggering error conditions, enabling more reliable access to archived data.

[coderabbitai]

Walkthrough

The SyncBatchReadForArchiveCallback function's KEY_NOT_FOUND branch was modified to treat missing keys as deleted entries. Instead of logging an error, it now logs at INFO level, constructs a tombstone value, adds it to results, and continues processing rather than aborting.

Changes

Cohort / File(s)	Change Summary
Error Handling Refactor data_store_service_client_closure.cpp	Modified KEY_NOT_FOUND branch in SyncBatchReadForArchiveCallback to log at INFO level and create tombstone values for missing keys instead of treating them as errors.

Sequence Diagram(s)

sequenceDiagram
    participant Callback as SyncBatchReadForArchiveCallback
    participant Handler as Error Handler

    rect rgb(200, 220, 255)
    Note over Callback,Handler: Before: KEY_NOT_FOUND Path
    Callback->>Handler: Key not found
    Handler->>Handler: Log error (commented code)
    Handler->>Callback: Abort/Error state
    end

    rect rgb(220, 255, 220)
    Note over Callback,Handler: After: KEY_NOT_FOUND Path
    Callback->>Handler: Key not found
    Handler->>Handler: Log INFO level
    Handler->>Handler: Construct tombstone value
    Handler->>Handler: Add to result
    Handler->>Handler: Update read count
    Handler->>Callback: Continue normal processing
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A missing key, no longer feared,
Becomes a tombstone, clean and clear!
No error logs, just INFO's light,
We process on with all our might. 🪦

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 pull request title "Use "INFO" level to print "key not found" log in SyncBatchReadForArchiveCallback" is clearly and specifically related to the changeset. The summary confirms that the KEY_NOT_FOUND branch was changed to log at INFO level instead of error level in the specified function. The title accurately captures this logging behavior change and is appropriately scoped—it doesn't need to describe the accompanying behavioral changes (like constructing tombstone values), as the instructions acknowledge that titles don't have to cover every detail. The title is concise, specific to the function, and provides clear value to a teammate reviewing commit history.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch print_key_not_found

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c78d788 and 94a7462.

📒 Files selected for processing (1)

• data_store_service_client_closure.cpp (1 hunks)

🔇 Additional comments (1)

data_store_service_client_closure.cpp (1)

66-81: Behavior change confirmed: treat KEY_NOT_FOUND as deleted entry instead of error.

The change is consistent with other callbacks in the same file (FetchRecordCallback, FetchSnapshotCallback, FetchTableStatisticsCallback all handle KEY_NOT_FOUND similarly by treating it as a deleted state with SetFinish(0) rather than setting an error). However, this represents a significant semantic shift for the caller at line 2340:

• Old behavior: Missing keys would likely fail the operation (error_code set, caller returns false)
• New behavior: Missing keys are treated as deleted entries, operation continues successfully

This means the archive read loop will now silently succeed with tombstone records for missing keys instead of aborting. Verify this aligns with the intended semantics for CopyBaseToArchive and that callers properly handle deleted entries in the results.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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