Fix REQUESTED_TABLE_NOT_EXISTS error#337
Conversation
WalkthroughThis PR adds documentation and expands error-handling logic in transaction operations. A descriptive comment clarifies behavior when schema information is unavailable during DDL commits. Error handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 0
🧹 Nitpick comments (1)
tx_service/include/cc/catalog_cc_map.h (1)
2418-2422: Early return creates unreachable code at lines 2435-2439.The new early return for
Deletedstatus (lines 2418-2422) makes the subsequent check at lines 2435-2439 unreachable for theDeletedcase. The early return is beneficial as it avoids unnecessary write-lock checks, but the redundant check should be removed.🔎 Proposed refactor to remove redundant check
const auto keylock = catalog_cce->GetKeyLock(); if (keylock != nullptr && keylock->HasWriteLock()) { DLOG(WARNING) << "InitCcm failure, target table: " << table_name.StringView() << " has write lock on catalog_cc_map, which means, it is " "being modified on this shard."; return InitCcmResult::Failure(CcErrorCode::READ_CATALOG_CONFLICT); } -RecordStatus payload_status = catalog_cce->PayloadStatus(); -if (payload_status == RecordStatus::Deleted) -{ - return InitCcmResult::Failure( - CcErrorCode::REQUESTED_TABLE_NOT_EXISTS); -} - -assert(payload_status == RecordStatus::Normal); +assert(catalog_cce->PayloadStatus() == RecordStatus::Normal); CatalogRecord *catalog_rec = catalog_cce->payload_.cur_payload_.get();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tx_service/include/cc/catalog_cc_map.h
🧰 Additional context used
🧬 Code graph analysis (1)
tx_service/include/cc/catalog_cc_map.h (1)
tx_service/include/error_messages.h (1)
CcErrorCode(237-440)
🔇 Additional comments (1)
tx_service/include/cc/catalog_cc_map.h (1)
1233-1239: LGTM! Clean short-circuit for deleted catalog entries.The early return when the payload status is Deleted is appropriate for the Read path, preventing unnecessary processing and clearly signaling that the catalog entry doesn't exist.
yi-xmu
force-pushed
the
fix_table_not_exist
branch
from
1c1ad76 to
39c542f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tx_service/src/tx_operation.cpp (1)
302-332: Assertion update forREQUESTED_TABLE_NOT_EXISTSlooks correctExtending the
assertto includeCcErrorCode::REQUESTED_TABLE_NOT_EXISTSkeeps the debug-time expectations aligned with the errors the handler can now return, without changing runtime behavior. The retry/abort logic below is unchanged and will treat this condition consistently with the other retriable metadata errors.If this error is specifically meant to represent the DDL-commit window for range-partitioned tables (as implied by the new comment in
cc_request.h), consider expanding the nearby comment here to mentionREQUESTED_TABLE_NOT_EXISTSas part of that DDL path to avoid future confusion for readers.tx_service/include/cc/cc_request.h (1)
172-181: Clarify that this error is not exclusively tied to DDL commit and is caller-dependentThe added comment is useful, but it currently reads as if
schema_ == nullptronly ever happens during a DDL commit phase and that every such request is retried viaReadLocalOperation::Forward(). In practice:
schema_ == nullptrcan also represent a genuinely dropped/non‑existent table (likely withdirty_schema_ == nullptr), not just an in‑progress DDL.TemplatedCcRequest::Executeis shared by many request types; only some are driven byReadLocalOperation::Forward(). For others, retry behavior depends on their own callers.To avoid misleading future readers, consider softening the wording to “can occur” and framing
ReadLocalOperation::Forward()as an example of a caller that treatsREQUESTED_TABLE_NOT_EXISTSas retriable, rather than implying that all callers do.✏️ Suggested comment tweak
- // This error occurs when a DDL transaction is in its + // This error can occur when a DDL transaction is in its // commit phase: commit operations execute sequentially // across all shards, but the dirty schema commit (which // updates the table catalog entry in the localccshard) // only happens on the last shard. Consequently, // concurrent requests may encounter `schema_ == // nullptr` because the schema update hasn't been - // applied to the localccshard yet. The request will be - // automatically retried in - // `ReadLocalOperation::Forward()`. + // applied to the localccshard yet. In that case, + // callers should treat `REQUESTED_TABLE_NOT_EXISTS` + // as a retriable error (for example, + // `ReadLocalOperation::Forward()` does this).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tx_service/include/cc/cc_request.htx_service/src/tx_operation.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T04:30:07.884Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 149
File: include/cc/cc_request.h:1876-1927
Timestamp: 2025-10-20T04:30:07.884Z
Learning: ScanNextBatchCc in include/cc/cc_request.h is used only for hash-partition scans; range-partition scans are handled by ScanSliceCc.
Applied to files:
tx_service/include/cc/cc_request.h
2 participants
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.