dbms/src/Storages/StorageDisaggregatedColumnar.cpp (2)

95-107: ⚡ Quick win

Log the original cleanup exception.

This broad catch (...) drops the exception details from clear_shared_snap_access_by_start_ts, so failures here become almost impossible to diagnose. tryLogCurrentException(log, "...") is the project-standard way to preserve that context in cleanup paths.

♻️ Suggested change

         catch (...)
         {
-            try
-            {
-                LOG_WARNING(log, "clear shared snapaccess cache failed, start_ts={}", start_ts);
-            }
-            catch (...)
-            {
-            }
+            tryLogCurrentException(log, fmt::format("clear shared snapaccess cache failed, start_ts={}", start_ts));
         }

As per coding guidelines, Use tryLogCurrentException(log, "context") in broad catch (...) paths to avoid duplicated exception-formatting code.

🤖 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 `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` around lines 95 - 107,
The catch-all after calling clear_shared_snap_access_by_start_ts(start_ts,
proxy_ptr) drops the original exception; replace the inner LOG_WARNING/empty
catch with a call to tryLogCurrentException(log, "clear shared snapaccess cache
failed, start_ts={}", start_ts) (or at minimum tryLogCurrentException(log,
"clear shared snapaccess cache failed, start_ts=" + toString(start_ts))) so the
original exception context is preserved; update the catch (...) block in
StorageDisaggregatedColumnar.cpp accordingly to call tryLogCurrentException(log,
...) instead of swallowing the exception.

Source: Coding guidelines

---

675-675: ⚡ Quick win

Use the fmt-style DB::Exception constructor here.

This new branch is still using the legacy (message, code) form while the surrounding paths already use the repository-standard Exception(ErrorCodes::..., "...") style.

♻️ Suggested change

-        throw Exception("lock error", ErrorCodes::COLUMNAR_SNAPSHOT_ERROR);
+        throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "lock error");

As per coding guidelines, Use DB::Exception for error handling with the fmt-style constructor: throw Exception(ErrorCodes::SOME_CODE, "Message with {}", arg);.

🤖 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 `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` at line 675, Replace the
legacy throw Exception("lock error", ErrorCodes::COLUMNAR_SNAPSHOT_ERROR) usage
with the repository-standard fmt-style constructor: throw
Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "lock error"); update the throw
site in StorageDisaggregatedColumnar (the throw of Exception referencing
ErrorCodes::COLUMNAR_SNAPSHOT_ERROR) so it uses the ErrorCodes-first signature
and follow the same fmt-style pattern as surrounding paths.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

dbms/src/Storages/StorageDisaggregatedColumnar.cpp (1)

668-680: ⚡ Quick win

Use the repo-standard DB::Exception constructor here.

These throws reverse the expected (error_code, format, ...) order. Please switch them to the fmt-style form so they do not depend on a legacy overload.

Suggested cleanup

-        throw Exception("lock error", ErrorCodes::COLUMNAR_SNAPSHOT_ERROR);
+        throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "lock error");
...
-            throw Exception("read_block failed in tiflash-proxy", ErrorCodes::LOGICAL_ERROR);
+            throw Exception(ErrorCodes::LOGICAL_ERROR, "read_block failed in tiflash-proxy");

As per coding guidelines, **/*.cpp: Use DB::Exception for error handling with the fmt-style constructor: throw Exception(ErrorCodes::SOME_CODE, "Message with {}", arg);

Also applies to: 1250-1254

🤖 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 `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` around lines 668 - 680,
Replace legacy Exception throws that pass message then code with the
repo-standard DB::Exception fmt-style constructor; specifically in
StorageDisaggregatedColumnar.cpp where the LockedError handling uses throw
Exception("lock error", ErrorCodes::COLUMNAR_SNAPSHOT_ERROR) (the block that
parses lock_info, calls cluster->lock_resolver->resolveLocks and logs
before_expired) change it to throw
Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "lock error") and apply the same
conversion for the similar throw in the other location referenced around the
1250–1254 area so all Exception instantiations use the (ErrorCodes::..., "fmt
{}", args...) form.

Source: Coding guidelines

dbms/src/Storages/StorageDisaggregatedColumnar.h (2)

67-85: 🏗️ Heavy lift

Please add automated coverage for the one-shot reader-slot lifecycle.

This header introduces a concurrency-sensitive state machine (NotStarted / Creating / Ready / Failed / Consumed) plus shared vs fixed reader acquisition, but the PR summary says validation was manual only. I’d want focused coverage for failure wakeups, double-consume rejection, and prefetch-vs-inline materialization before this leaves hold.

Also applies to: 114-126, 204-205

🤖 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 `@dbms/src/Storages/StorageDisaggregatedColumnar.h` around lines 67 - 85, Add
automated unit tests that exercise the RNProxyReaderSlot lifecycle and its
RNProxyReaderMaterializeState transitions: write tests that (1) simulate a
materialization failure by setting slot.exception and state=Failed and verify
waiting threads are woken via the slot.cv and that the exception is propagated
to callers; (2) validate double-consume rejection by acquiring slot.reader once
(state==Ready -> consume) then attempting a second acquire and asserting it
fails or is rejected and state becomes Consumed; and (3) cover
prefetch-vs-inline materialization by driving both code paths that populate
slot.reader directly (inline) and that prefetch/async-create (Creating -> Ready)
and assert correct final states, proper reader ownership in slot.reader, and
that the destructor (~RNProxyReaderSlot) does not leak or leave threads blocked.
Use RNProxyReaderSlot, RNProxyReaderMaterializeState, reader, exception, mutex,
and cv symbols to locate and exercise the logic.

---

59-65: 🏗️ Heavy lift

Normalize the new header API to TiFlash naming/types now.

The added public contract exposes snake_case identifiers and platform-width size_t indices. Since this header is new surface area for the columnar read path, it is much cheaper to align it with the project conventions before more callers land. As per coding guidelines, **/*.{cpp,h,hpp}: Use explicit width types from dbms/src/Core/Types.h: UInt8, UInt32, Int64, Float64, String and Method and variable names should use camelCase.

Also applies to: 118-156, 176-186, 210-215, 269-273

🤖 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 `@dbms/src/Storages/StorageDisaggregatedColumnar.h` around lines 59 - 65, The
public struct and related APIs must follow project naming and explicit-width
type conventions: rename RNProxyReaderPlan member identifiers from snake_case to
camelCase (e.g., region_id -> regionId, region_ver -> regionVersion,
region_conf_ver -> regionConfVer, physical_table_ranges -> physicalTableRanges)
and replace any platform-width/index types (e.g., size_t or plain int) with the
explicit types from dbms/src/Core/Types.h (UInt8/UInt32/Int64/UInt64/etc.) as
appropriate; update the RNProxyReaderPlan definition and all other affected
structs/functions referenced in the comment (the other blocks at 118-156,
176-186, 210-215, 269-273) to use the same camelCase names and fixed-width types
so public API matches TiFlash conventions.

Source: Coding guidelines