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: [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]
