fix: keep the classification when a vortex cell load fails - #610
Open
xiaofan-luan wants to merge 1 commit into
Open
fix: keep the classification when a vortex cell load fails#610xiaofan-luan wants to merge 1 commit into
xiaofan-luan wants to merge 1 commit into
Conversation
`VortexTranslater::get_cells` threw `std::runtime_error(status.ToString())`. The status arriving there is fully classified on the object-store path -- S3FileSystem's ErrorToStatus attaches an ExtendStatusDetail, and both hops up (FillVortexRangeFile, the loader lambda) propagate it untouched via ARROW_*_RAISE -- so stringifying it destroyed the one thing the caller needed. A throttled object store during a vortex cache miss therefore surfaced as an untyped exception, landed on the "unclassified means permanent" fallback, and was never retried. Throw `ToSegcoreError(status)` instead. `milvus::SegcoreError` is already an exception type carrying a `milvus::ErrorCode`, so the classification survives the throw. The exception itself stays: `Translator::get_cells` returns a vector and milvus-common's caching layer reports load failures through exceptions -- that is the interface's contract and not ours to change here. What we throw is ours. Regression test drives a real translator over a filesystem whose reads fail with a classified throttle and asserts the caught exception carries StorageTransientError. Red/green verified: with the fix reverted the test fails in the `catch (const std::exception&)` branch with "load failure lost its type", and no other test changes verdict -- blast radius is exactly the one test written to catch it. Worth noting what the red run prints: the reverted message still *contains* "Detail: StorageTransientThrottling: SlowDown". The classification was never missing from the text, it was just unbranchable. That is the whole bug. Part of milvus-io#609. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xiaofan-luan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
==========================================
- Coverage 76.00% 75.99% -0.02%
==========================================
Files 168 169 +1
Lines 16688 16937 +249
Branches 2511 2535 +24
==========================================
+ Hits 12684 12871 +187
- Misses 4004 4066 +62
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jiaqizho
reviewed
Jul 31, 2026
| const auto& cell_metas = *cell_metas_; | ||
| for (auto cid : cids) { | ||
| if (cid < 0 || static_cast<size_t>(cid) >= cell_metas.size()) { | ||
| throw std::out_of_range(fmt::format("Vortex cell id {} is out of range, num_cells={}", cid, cell_metas.size())); |
Collaborator
There was a problem hiding this comment.
Should this throw also be handled?
jiaqizho
reviewed
Jul 31, 2026
| VortexTranslater::estimated_byte_size_of_cell(milvus::cachinglayer::cid_t cid) const { | ||
| const auto& cell_metas = *cell_metas_; | ||
| if (cid < 0 || static_cast<size_t>(cid) >= cell_metas.size()) { | ||
| throw std::out_of_range(fmt::format("Vortex cell id {} is out of range, num_cells={}", cid, cell_metas.size())); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
VortexTranslater::get_cellsthrew the status as a string:The status arriving there is fully classified on the object-store path —
S3FileSystem'sErrorToStatusattaches anExtendStatusDetail, and both hops up (FillVortexRangeFile, the loader lambda) propagate it untouched viaARROW_*_RAISE. Stringifying it destroyed the one thing the caller needed.Consequence: a throttled object store during a vortex cache miss surfaced as an untyped exception, hit the "unclassified means permanent" fallback, and was never retried.
Fix
milvus::SegcoreErroris already an exception type carrying amilvus::ErrorCode, so the classification survives the throw.The exception itself stays.
Translator::get_cellsreturns a vector, so milvus-common's caching layer reports load failures through exceptions — that is the interface's contract and not this PR's to change. What we throw is ours.Verification
Regression test drives a real translator over a filesystem whose reads fail with a classified throttle, and asserts the caught exception carries
StorageTransientError.Red/green verified rather than assumed:
*Vortex*80 passed / 0 failed / 15 skipped (pre-existing cloud gates)catch (const std::exception&)branch:load failure lost its type, got: IOError: SlowDown...Blast radius with the fix reverted is exactly the one test written to catch it — nothing else was depending on the stringified-throw behaviour.
One detail from the red run that is worth stating, because it is the shape of this whole bug class: the reverted message still contains
Detail: StorageTransientThrottling: SlowDown. The classification was never missing from the text. It was just unbranchable.Test harness note
The fake source file implements
arrow::io::RandomAccessFiledirectly rather than deriving fromarrow::io::BufferReader.BufferReaderinheritsRandomAccessFileConcurrencyWrapper<BufferReader>, which declares bothReadAtoverloadsfinaland dispatches through CRTP to a non-virtualDoReadAt— a subclass cannot intercept a read there at all, and aDoReadAtoverride would silently never be called. SimilarlyThrottledFileSystemwraps the fixture's rooted filesystem instead of subclassingLocalFileSystem, which would resolve the relative path against the process CWD.Scope
This is one of three sites in the same class, tracked in #609. The other two are left for separate PRs because they need a decision rather than a mechanical change:
cpp/src/packed/reader.cpp:88is the deprecated throwingPackedRecordBatchReaderconstructor. If it is going away, deleting it beats reclassifying it.cpp/src/packed/column_group.cpp:64usesresult.status().message(), which drops even the arrowStatusCode. The condition isarrow::Table::FromRecordBatchesfailing on a schema mismatch across batches — our own invariant, soPackedUnexpectedrather than a passthrough.Related: #603 defines the taxonomy this fix preserves.
🤖 Generated with Claude Code
https://claude.ai/code/session_011HDgMok3nR8ZNugKWWQQK2