github-actions[bot] commented on code in PR #68482:
URL: https://github.com/apache/doris/pull/68482#discussion_r4091217049
##########
be/src/storage/rowset/beta_rowset.cpp:
##########
@@ -255,8 +255,8 @@ Status
BetaRowset::load_segments(std::vector<segment_v2::SegmentSharedPtr>* segm
Status BetaRowset::load_segment(int64_t seg_id, OlapReaderStatistics* stats,
segment_v2::SegmentSharedPtr* segment,
const io::IOContext* io_ctx) {
- return
load_segment(_rowset_meta->segment_ref(_rowset_meta->position_of(seg_id)),
stats,
- segment, io_ctx);
+ auto pos = DORIS_TRY(_rowset_meta->position_of(seg_id));
Review Comment:
This only makes the lookup-after-segcompaction interleaving fail safely. If
the async task opens the original segment first, it can add a
`TEMP_VERSION_COMMON` deletion after segcompaction has already converted the
source bitmap. `build()` waits for that task only after conversion, and
`_close_file_writers()` then removes the late source key and merges the stale
converted bitmap, so a sequence-column MoW load can still commit with the
lower-sequence row visible. Please order each source segment's bitmap task
before conversion/removal (or recompute conversion from the final bitmap after
the token wait), and add a sync-point test for
open-before-convert/add-after-convert.
##########
be/src/storage/rowset/rowset_meta.cpp:
##########
@@ -349,16 +349,22 @@ void RowsetMeta::set_segment_ids(const
std::vector<int64_t>& segment_ids) {
_validate_segment_ids();
}
-size_t RowsetMeta::position_of(int64_t seg_id) const {
+Result<size_t> RowsetMeta::position_of(int64_t seg_id) const {
DORIS_CHECK_GE(seg_id, 0);
if (!has_segment_ids()) {
- DORIS_CHECK_LT(seg_id, num_segments());
+ if (seg_id >= num_segments()) {
+ return ResultError(
+ Status::Error<NOT_FOUND>("segment {} not found in rowset
{}, tablet {}", seg_id,
Review Comment:
`NOT_FOUND` is declared in `doris::ErrorCode`, but this translation unit
never imports that namespace (unlike the other storage `.cpp` files that use
`Error<NOT_FOUND>`). Both new template arguments therefore fail in a
standalone/non-unity compile. Please use `ErrorCode::NOT_FOUND` (or
`Status::NotFound(...)`) for both branches.
##########
be/test/storage/rowset/rowset_meta_test.cpp:
##########
@@ -635,9 +635,29 @@ TEST_F(RowsetMetaTest, TestSegmentIdsAccessors) {
EXPECT_EQ(rowset_meta.segment_id(0), 0);
EXPECT_EQ(rowset_meta.segment_id(1), 2);
EXPECT_EQ(rowset_meta.segment_id(2), 5);
- EXPECT_EQ(rowset_meta.position_of(0), 0);
- EXPECT_EQ(rowset_meta.position_of(2), 1);
- EXPECT_EQ(rowset_meta.position_of(5), 2);
+ EXPECT_EQ(TEST_TRY(rowset_meta.position_of(0)), 0);
+ EXPECT_EQ(TEST_TRY(rowset_meta.position_of(2)), 1);
+ EXPECT_EQ(TEST_TRY(rowset_meta.position_of(5)), 2);
+}
+
+TEST_F(RowsetMetaTest, TestPositionOfMissingSegment) {
+ RowsetMeta rowset_meta;
+ ASSERT_TRUE(rowset_meta.init_from_json(_json_rowset_meta));
+
+ rowset_meta.set_num_segments(0);
Review Comment:
These three new `is<NOT_FOUND>()` checks have the same namespace issue as
the production code: this test file never imports `doris::ErrorCode`, so the
unit-test TU cannot resolve `NOT_FOUND`. Please qualify all three as
`ErrorCode::NOT_FOUND` (or add an intentional using-declaration); fixing only
`rowset_meta.cpp` will still leave the tests uncompilable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
