Repository navigation
branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) - #68820
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…es and dynamic-partition logic (apache#63551) Pick apache#63551 to branch-4.1. - SchemaChangeHandler: avoid the race between ALTER TABLE property updates and DynamicPartitionScheduler dropping partitions concurrently. - LoadManager / LoadProcessor: add diagnostic logs for the InsertLoadJob jobId mismatch path that shows up as all-zero JobDetails in SHOW LOAD. - regression-test: use 'admin set all frontends config' in test_manager_interface_1, clean up the reserved-word 'order' table in test_doris_jdbc_catalog, and raise max_remote_file_system_cache_num in the P0 pipeline fe.conf. The check_hash_bucket_table, scanner_profile, test_temp_table and test_backup_restore_colocate fixes from apache#63551 are already present on branch-4.1, so they are not duplicated here.
854f0d1 to
2e82f2d
Compare
hubgeter
changed the title
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Complete static review of PR #68820 at head 2e82f2d. I found three new P2 issues in the inline comments. No existing inline P0/P1 thread applies; the PR has no prior inline comments.
Critical checkpoints:
- Goal and scope: The new predicate correctly avoids per-partition tablet-meta dispatch for catalog-only changes such as
partition.retention_count, and the added test cleanup/config changes are localized. The catch added for concurrent drops still has the two correctness problems below. The load diagnostic change warns on a normal transaction INSERT path. No new regression test proves the concurrent drop/recovery or transaction-warning behavior. - Concurrency and lifecycle:
DynamicPartitionSchedulercan remove partitions while the ALTER is outside its table read lock and waiting on BE tasks. The new lookup reads a mutable TreeMap without that lock, and soft-dropped partitions retain tablets for recovery. I traced the snapshot, tablet task, table-property commit, recycle, and recovery steps; no new lock-order or cross-TU initialization issue was found. - Configuration, compatibility, and parallel paths: No production config, FE-to-BE variable, function symbol, storage format, or protocol changed. The FE p0 cache limit is a bounded test setting. The all-FE timeout test uses the supported broadcast path and restores the value. The cloud schema-change override already treats retention-only changes as catalog metadata. The BE-dispatch predicate covers every optional field passed by this call; omitted properties previously produced no-op tablet tasks.
- Persistence, writes, and failure handling:
modifyTablePropertiesstill journals under the table write lock, but the new catch can report success before a recycled partition's tablet metadata was updated. This PR does not change transaction data writes or visibility rules. Failed BE task status still propagates unless the new unsynchronized absence check suppresses it. No separate master-failover or edit-log replay defect was substantiated. - Observability and performance: The guarded DEBUG logs are safe for null query IDs. The unconditional new WARN is noisy on expected transaction-model INSERTs. Skipping all-sentinel BE tasks removes redundant work; no additional cache, CPU, memory, or metrics issue was substantiated.
- Tests and other coverage: I reviewed all six changed paths and related FE/BE flows, including JDBC catalog context and the qualified
ordertable cleanup. No expected-result file changed. Builds and tests were prohibited by the review contract, so validation is static inspection only. There is no additional user-provided focus point. Two normal coverage passes and a separate risk-focused pass converged with no new independent findings in round 2; the final changed-file and candidate sweep found no unresolved point.
| // The partition may have been dropped concurrently (e.g., by | ||
| // DynamicPartitionScheduler). It is safe to skip the meta dispatch | ||
| // for a partition that no longer exists. | ||
| if (olapTable.getPartition(partition.getName()) == null) { |
There was a problem hiding this comment.
[P2] Lock this partition lookup before deciding to suppress a BE failure. getPartition(name) reads OlapTable.nameToPartition, a plain TreeMap, while the scheduler can remove another partition under the table write lock. If P's BE task fails or times out and that removal rotates the tree during this unlocked read, getPartition(P) can return null even though P is still active; continue then commits the new FE property while P's BE metadata is stale. Recheck the formal partition under the table read lock and only skip if the original partition is truly gone.
| if (olapTable.getPartition(partition.getName()) == null) { | ||
| LOG.info("partition {} of table {} was dropped concurrently, " | ||
| + "skip updating its properties", partition.getName(), olapTable.getName()); | ||
| continue; |
There was a problem hiding this comment.
[P2] Do not treat a soft-dropped partition as disposable. If the scheduler drops P after this ALTER snapshots it but before updatePartitionProperties looks it up, that method throws before sending any tablet task. This continue now commits a new table compaction_policy anyway. Scheduler drops are non-force, so P's tablets remain in the recycle bin; RECOVER PARTITION reattaches them without sending the new policy. The post-snapshot case previously failed instead of reporting a successful ALTER with stale recoverable tablets. Preserve failure or reconcile those retained tablets before committing.
| // SHOW LOAD's JobDetails (ScannedRows / LoadBytes / All backends) will be all zero. | ||
| // Logging this at WARN so CI failures of the form | ||
| // "test_insert_statistic: expected:<N> but was:<0>" can be diagnosed directly. | ||
| LOG.warn("recordFinishedLoadJob: jobId={} not found in idToLoadJob, creating a new " |
There was a problem hiding this comment.
[P2] Avoid warning for normally unregistered transaction INSERTs. OlapTxnInsertExecutor calls the default superclass constructor (needRegister=false), while the master gives it a positive job ID and inherited afterExec still calls recordFinishedLoadJob. Each successful nonempty transaction INSERT ... SELECT routed through that executor therefore reaches this WARN despite no lost registration, filling FE warning logs and obscuring actual missing-job incidents. Gate WARN to paths that promise early registration or keep the expected transaction path at DEBUG.
FE Regression Coverage ReportIncrement line coverage |
pick #63551