Fix handling putall failure

[thweetkomputer]

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling for data operations with proper resource cleanup on failures.

• Refactor

  • Minor code organization and style improvements.

[coderabbitai]

Walkthrough

Modified data_store_service_client.cpp to relocate a header include directive, add resource cleanup logic to PutAll when partition operations fail, apply stylistic reformatting to UpsertTable, and correct namespace closure annotation from EloqD to EloqDS.

Changes

Cohort / File(s)	Summary
Header Include Relocation data_store_service_client.cpp	Moved data_store_service_client.h include to the top of the file and removed redundant inclusion elsewhere.
Error Path Cleanup data_store_service_client.cpp	Added Clear and Free calls in PutAll to release callback data resources when a partition operation fails, preventing resource leaks on error paths.
Stylistic Reformatting data_store_service_client.cpp	Minor formatting adjustment to lambda capture list for index_add_names_ in UpsertTable without functional changes.
Namespace Annotation data_store_service_client.cpp	Updated closing namespace comment from EloqD to EloqDS for consistency.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Pay special attention to the PutAll error-path cleanup logic to ensure all callback data structures are properly deallocated and no double-frees occur
• Verify that the cleanup sequence matches the allocation strategy used elsewhere in the codebase

Possibly related PRs

• store_handler#83: Introduces per-partition/partition-callback batching in PutAll, which is complemented by the error-path cleanup added in this PR.

Suggested reviewers

• lzxddz

Poem

🐰 A file is tidied, headers aligned,
Resources freed when errors unwind,
Namespaces corrected with care,
Cleanup code shows we're fair!
The store runs cleaner everywhere! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'fix' is extremely vague and does not convey any meaningful information about the specific changes made in the pull request.	Replace with a descriptive title that reflects the main change, such as 'Fix resource cleanup in PutAll on partition failure' or 'Fix batch write zero-copy handling'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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 fix_batch_write_zc

---

📜 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 1a8e912 and bb5ee12.

📒 Files selected for processing (1)

• data_store_service_client.cpp (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

data_store_service_client.cpp (1)

bigtable_handler.cpp (2)

• InitTableRanges (2361-2396)
• InitTableRanges (2361-2362)

🔇 Additional comments (4)

data_store_service_client.cpp (4)

22-23: LGTM: Proper header include placement.

Including the corresponding header file first after the copyright notice is standard practice and ensures the header is self-contained.

---

448-452: Excellent fix: Prevents resource leak on partition failure.

This change correctly adds cleanup for pooled callback_data objects when a partition fails, matching the cleanup already present in the success path (lines 457-461). Without this, failing partitions would leak callback data objects from the pool, eventually exhausting resources under error conditions.

The fix is correctly placed after waiting for all async operations to complete (lines 432-437) and before returning the error status.

---

3378-3387: LGTM: Improved lambda formatting.

The reformatting improves readability without changing functionality. The lambda correctly captures this and new_table_schema to call InitTableRanges for each index.

---

4761-4761: LGTM: Namespace comment correction.

Corrects the closing namespace comment to match the actual namespace name EloqDS declared at line 52.

---

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