github-actions[bot] commented on code in PR #64560: URL: https://github.com/apache/doris/pull/64560#discussion_r3449688847
########## be/src/storage/rowset/beta_rowset_writer.cpp: ########## @@ -541,6 +568,12 @@ Status BetaRowsetWriter::_find_longest_consecutive_small_segment( Review Comment: The gate prevents the immediate race, but it also drops the compaction attempt permanently when all triggers happen while delete bitmap work is still pending. `_segcompaction_if_necessary()` is only called from `add_segment()`: if this check breaks for segment 0 on each trigger, `build()` later waits for `_calc_delete_bitmap_token`, then `_close_file_writers()` only calls `_segcompaction_rename_last_segments()` when `is_segcompacted()` is already true. There is no retry after the bitmaps become ready, so a fast MoW load can keep every small segment un-compacted and then fail the final segment-count check once it exceeds `max_segment_num_per_rowset`, even though segment compaction is enabled. Please re-run the selection after the token wait, or make the pending-bitmap case wait/reschedule instead of just returning with no future trigger. ########## regression-test/suites/fault_injection_p0/test_mow_load_delete_bitmap_segcompaction_race.groovy: ########## @@ -0,0 +1,100 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_mow_load_delete_bitmap_segcompaction_race", "nonConcurrent") { + if (isCloudMode()) { + logger.info("skip test in cloud mode") + return + } + + GetDebugPoint().clearDebugPointsForAllBEs() + def segcompactionBatchSize = get_be_param("segcompaction_batch_size") + def dorisScannerRowBytes = get_be_param("doris_scanner_row_bytes") + def enableAdaptiveBatchSize = get_be_param("enable_adaptive_batch_size") + + onFinish { + GetDebugPoint().clearDebugPointsForAllBEs() + set_original_be_param("segcompaction_batch_size", segcompactionBatchSize) + set_original_be_param("doris_scanner_row_bytes", dorisScannerRowBytes) + set_original_be_param("enable_adaptive_batch_size", enableAdaptiveBatchSize) + } + + sql "DROP TABLE IF EXISTS test_mow_load_delete_bitmap_segcompaction_race FORCE" + sql """ + CREATE TABLE test_mow_load_delete_bitmap_segcompaction_race ( + k1 int NOT NULL, + c1 int, + c2 int + ) + UNIQUE KEY(k1) + DISTRIBUTED BY HASH(k1) BUCKETS 1 + PROPERTIES ( + "enable_unique_key_merge_on_write" = "true", + "disable_auto_compaction" = "true", + "replication_num" = "1" + ) + """ + + sql """ + INSERT INTO test_mow_load_delete_bitmap_segcompaction_race VALUES + (100001, 1, 1), + (100002, 2, 2), + (100003, 3, 3) + """ + sql "sync" + + set_be_param.call("segcompaction_batch_size", "2") + set_be_param.call("doris_scanner_row_bytes", "1") + set_be_param.call("enable_adaptive_batch_size", "false") + GetDebugPoint().enableDebugPointForAllBEs("MemTable.need_flush") + GetDebugPoint().enableDebugPointForAllBEs( + "BetaRowsetWriter.generate_delete_bitmap.sleep_before_build_tmp", + [sleep_ms: 2000]) + GetDebugPoint().enableDebugPointForAllBEs( + "BetaRowsetWriter.add_segment.sleep_before_segcompaction", + [segment_id: 1, sleep_ms: 200]) + + String content = "" + (1..4096).each { + content += "${it},${it},${it}\n" + } + content += content + + streamLoad { + table "test_mow_load_delete_bitmap_segcompaction_race" + set "column_separator", "," + inputStream new ByteArrayInputStream(content.getBytes()) + time 120000 + + check { result, exception, startTime, endTime -> + if (exception != null) { + throw exception + } + logger.info("stream load result: ${result}") + def json = parseJson(result) + assertEquals("success", json.Status.toLowerCase()) Review Comment: This assertion is not enough to prove the regression exercised the race. The input is 4096 rows duplicated, but the final expected count is 4099, which would also be produced if only the first half of the stream load landed on top of the three seed rows; `dup_key_count` would still be 0 in that case. The test also never verifies that the load actually created the intended multi-segment/segcompaction setup. Please assert `NumberTotalRows == 8192` and `NumberFilteredRows == 0` in the stream-load result, and add a segment/compaction-state check like the nearby multi-segment tests so this cannot pass without hitting the path this PR fixes. -- 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]
