You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "hubgeter (via GitHub)" <gi...@apache.org> on 2026/10/09 06:59:07 UTC
[PR] branch-4.1: [fix](alter) avoid race between ALTER table properties and DynamicPartitionScheduler (#63551) [doris]
hubgeter opened a new pull request, #68820:
URL: https://github.com/apache/doris/pull/68820
pick #63551
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #68820:
URL: https://github.com/apache/doris/pull/68820#issuecomment-6078595049
# FE UT Coverage Report
Increment line coverage ` 7.69% (2/26)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/68820_2e82f2d98f9103a2d210692f4fcf71a913e6248f/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/68820_2e82f2d98f9103a2d210692f4fcf71a913e6248f/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) [doris]
Posted by "hubgeter (via GitHub)" <gi...@apache.org>.
hubgeter commented on PR #68820:
URL: https://github.com/apache/doris/pull/68820#issuecomment-6076173021
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #68820:
URL: https://github.com/apache/doris/pull/68820#issuecomment-6080351332
# FE Regression Coverage Report
Increment line coverage ` 26.92% (7/26)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/68820_2e82f2d98f9103a2d210692f4fcf71a913e6248f_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/68820_2e82f2d98f9103a2d210692f4fcf71a913e6248f_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei merged PR #68820:
URL: https://github.com/apache/doris/pull/68820
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on PR #68820:
URL: https://github.com/apache/doris/pull/68820#issuecomment-6079644488
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](alter) avoid race between ALTER table properties and DynamicPartitionScheduler (#63551) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #68820:
URL: https://github.com/apache/doris/pull/68820#issuecomment-6076074339
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://doris.apache.org/community/how-to-contribute/review-and-ci).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] branch-4.1: [fix](test)(dynamic-partition) fix some unstable test cases and dynamic-partition logic (#63551) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #68820:
URL: https://github.com/apache/doris/pull/68820#discussion_r4229549875
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2508,10 +2508,35 @@ public void updateTableProperties(Database db, String tableName, Map<String, Str
skip = Boolean.parseBoolean(skipWriteIndexOnLoad) ? 1 : 0;
}
- for (Partition partition : partitions) {
- updatePartitionProperties(db, olapTable.getName(), partition.getName(), storagePolicyId, isInMemory,
- null, compactionPolicy, timeSeriesCompactionConfig, skip,
- disableAutoCompaction, verticalCompactionNumColumnsPerGroup);
+ // Only iterate partitions when there are properties that actually need to be
+ // dispatched to each partition's tablets. Pure catalog-level metadata properties
+ // such as partition.retention_count do not require per-partition updates, and
+ // iterating over a stale partition snapshot can race with concurrent partition
+ // drops (e.g., by DynamicPartitionScheduler when retention_count or dynamic_partition
+ // is enabled) and fail with "Partition does not exist".
+ boolean needPerPartitionUpdate = isInMemory >= 0 || storagePolicyId >= 0
+ || compactionPolicy != null || !timeSeriesCompactionConfig.isEmpty()
+ || skip >= 0 || disableAutoCompaction >= 0
+ || verticalCompactionNumColumnsPerGroup >= 0;
+ if (needPerPartitionUpdate) {
+ for (Partition partition : partitions) {
+ try {
+ updatePartitionProperties(db, olapTable.getName(), partition.getName(),
+ storagePolicyId, isInMemory, null, compactionPolicy, timeSeriesCompactionConfig,
+ skip, disableAutoCompaction,
+ verticalCompactionNumColumnsPerGroup);
+ } catch (DdlException e) {
+ // 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) {
+ LOG.info("partition {} of table {} was dropped concurrently, "
+ + "skip updating its properties", partition.getName(), olapTable.getName());
+ continue;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/LoadManager.java:
##########
@@ -216,11 +216,25 @@ public void recordFinishedLoadJob(String label, long transactionId, String dbNam
LoadJob loadJob;
if (idToLoadJob.containsKey(jobId)) {
loadJob = idToLoadJob.get(jobId);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("recordFinishedLoadJob: reuse existing load job, jobId={}, label={}, dbId={}, jobType={}",
+ jobId, label, db.getId(), jobType);
+ }
if (loadJob instanceof InsertLoadJob) {
((InsertLoadJob) loadJob).setJobProperties(transactionId, tableId, createTimestamp,
failMsg, trackingUrl, firstErrorMsg, userInfo);
}
} else {
+ // The jobId received here does not exist in idToLoadJob. This means the InsertLoadJob
+ // that was registered during executor construction (and that accumulated BE-reported
+ // load statistics via updateJobProgress) is NOT the one we are about to snapshot here.
+ // A brand-new InsertLoadJob will be created below with an empty LoadStatistic, so
+ // 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 "
Review 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/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2508,10 +2508,35 @@ public void updateTableProperties(Database db, String tableName, Map<String, Str
skip = Boolean.parseBoolean(skipWriteIndexOnLoad) ? 1 : 0;
}
- for (Partition partition : partitions) {
- updatePartitionProperties(db, olapTable.getName(), partition.getName(), storagePolicyId, isInMemory,
- null, compactionPolicy, timeSeriesCompactionConfig, skip,
- disableAutoCompaction, verticalCompactionNumColumnsPerGroup);
+ // Only iterate partitions when there are properties that actually need to be
+ // dispatched to each partition's tablets. Pure catalog-level metadata properties
+ // such as partition.retention_count do not require per-partition updates, and
+ // iterating over a stale partition snapshot can race with concurrent partition
+ // drops (e.g., by DynamicPartitionScheduler when retention_count or dynamic_partition
+ // is enabled) and fail with "Partition does not exist".
+ boolean needPerPartitionUpdate = isInMemory >= 0 || storagePolicyId >= 0
+ || compactionPolicy != null || !timeSeriesCompactionConfig.isEmpty()
+ || skip >= 0 || disableAutoCompaction >= 0
+ || verticalCompactionNumColumnsPerGroup >= 0;
+ if (needPerPartitionUpdate) {
+ for (Partition partition : partitions) {
+ try {
+ updatePartitionProperties(db, olapTable.getName(), partition.getName(),
+ storagePolicyId, isInMemory, null, compactionPolicy, timeSeriesCompactionConfig,
+ skip, disableAutoCompaction,
+ verticalCompactionNumColumnsPerGroup);
+ } catch (DdlException e) {
+ // 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) {
Review 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.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org