morrySnow commented on code in PR #66488:
URL: https://github.com/apache/doris/pull/66488#discussion_r3765918854
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/AnalysisManager.java:
##########
@@ -548,6 +550,13 @@ public void updateTaskStatus(AnalysisInfo info,
AnalysisState taskState, String
if (MetricRepo.isInit) {
MetricRepo.COUNTER_STATISTICS_FAILED_ANALYZE_JOB.increase(1L);
}
+ // The job reached a terminal state: all tasks share the
job's
+ // partitionUpdateRows map, so clearing it here releases
the memory
+ // retained by every task record in the history at once.
The success
+ // path clears it inside updateTableStats.
+ if (job.partitionUpdateRows != null) {
+ job.partitionUpdateRows.clear();
Review Comment:
The clear here races with sibling task threads that are still executing.
When one task fails, `AnalysisJob.taskFailed` -> `updateTaskState(FAILED)`
marks ALL tasks of the job FAILED via `updateTaskStatus` and then calls
`task.cancel()` asynchronously (it only cancels the stmtExecutor; the task
thread keeps running). The last of these `updateTaskStatus` calls reaches this
branch and clears the shared map while the other task threads may still be
inside `doPartitionTable()` / `deleteNotExistPartitionStats()` writing to the
same map: `jobInfo.partitionUpdateRows.putIfAbsent(...)`
(BaseAnalysisTask.java:516/523) and
`jobInfo.partitionUpdateRows.remove(partId)` (OlapAnalysisTask.java:371).
Writes that land after the clear re-populate the map, and since
`analysisJobIdToTaskMap.remove(job.jobId)` happens right after, no later
`updateTaskStatus` will clear it again — so in failure/cancel scenarios the
memory this PR intends to release is still retained by the job/task history
records. T
he same applies to the analyze-timeout path (`BaseAnalysisTask.cancel()` ->
`updateTaskStatus(FAILED)` from the executor thread) and to
`handleKillAnalyzeJob`. Suggest clearing only after the sibling task threads
have actually terminated (e.g. in `AnalysisJob.taskFailed`/`deregisterJob`
after cancel), or making the task-side writers skip when the job has reached a
terminal state.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/AnalysisManager.java:
##########
@@ -992,11 +1006,17 @@ public void handleKillAnalyzeJob(KillAnalyzeJobCommand
killAnalyzeJobCommand) th
return;
}
checkPriv(anyTask);
- logKilled(analysisJobInfoMap.get(anyTask.getJobId()));
+ AnalysisInfo job = analysisJobInfoMap.get(anyTask.getJobId());
+ logKilled(job);
for (BaseAnalysisTask taskInfo : analysisTaskMap.values()) {
taskInfo.cancel();
logKilled(taskInfo.info);
}
+ // The job reached a terminal state: all tasks share the job's
partitionUpdateRows
+ // map, so clearing it here releases the memory retained by every task
record.
+ if (job.partitionUpdateRows != null) {
+ job.partitionUpdateRows.clear();
Review Comment:
`taskInfo.cancel()` is asynchronous: it only cancels the running
stmtExecutor and returns immediately, so sibling task threads may still be
inside `doPartitionTable()` / `deleteNotExistPartitionStats()` writing to
`job.partitionUpdateRows` (`putIfAbsent` at BaseAnalysisTask.java:516/523,
`remove` at OlapAnalysisTask.java:371) when this clear runs. A write landing
after the clear re-populates the map, and it is then never cleared again: since
`analysisJobIdToTaskMap.remove(jobId)` happened at the top of this method, the
`updateTaskStatus(FAILED)` calls triggered by `cancel()` all return early, and
the terminal-state clear in `updateTaskStatus` never runs either. So in the
kill scenario the memory is retained despite the clear, and the history records
can even expose a partially repopulated map. Consider clearing after the task
threads have actually stopped (e.g. in `AnalysisJob.deregisterJob`, which runs
after cancellation) rather than immediately after the async cancel loop.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/AnalysisManager.java:
##########
@@ -567,22 +576,27 @@ public void updateTaskStatus(AnalysisInfo info,
AnalysisState taskState, String
@VisibleForTesting
public void updateTableStats(AnalysisInfo jobInfo) {
- TableIf tbl = StatisticsUtil.findTable(jobInfo.catalogId,
jobInfo.dbId, jobInfo.tblId);
- TableStatsMeta tableStats = findTableStatsStatus(tbl.getId());
- if (tableStats == null) {
- updateTableStatsStatus(new TableStatsMeta(jobInfo.rowCount,
jobInfo, tbl));
- } else {
- tableStats.update(jobInfo, tbl);
- logCreateTableStats(tableStats);
- }
- if (jobInfo.jobColumns != null) {
- jobInfo.jobColumns.clear();
- }
- if (jobInfo.partitionNames != null) {
- jobInfo.partitionNames.clear();
- }
- if (jobInfo.partitionUpdateRows != null) {
- jobInfo.partitionUpdateRows.clear();
+ // Clear the shared maps even when the stats update fails, so the job
and all its
+ // task records release their memory once the job reaches a terminal
state.
+ try {
+ TableIf tbl = StatisticsUtil.findTable(jobInfo.catalogId,
jobInfo.dbId, jobInfo.tblId);
+ TableStatsMeta tableStats = findTableStatsStatus(tbl.getId());
+ if (tableStats == null) {
+ updateTableStatsStatus(new TableStatsMeta(jobInfo.rowCount,
jobInfo, tbl));
+ } else {
+ tableStats.update(jobInfo, tbl);
+ logCreateTableStats(tableStats);
+ }
+ } finally {
+ if (jobInfo.jobColumns != null) {
+ jobInfo.jobColumns.clear();
+ }
+ if (jobInfo.partitionNames != null) {
+ jobInfo.partitionNames.clear();
+ }
+ if (jobInfo.partitionUpdateRows != null) {
+ jobInfo.partitionUpdateRows.clear();
Review Comment:
Coverage gap: a job that is evicted from `analysisJobInfoMap` while still
running (the record-limit eviction in `replayCreateAnalysisJob` removes the
oldest entry regardless of its state) never reaches any terminal-state clear:
`updateTaskStatus` returns early at the `job == null` guard ("Job may get
deleted during execution"), and the shared `partitionUpdateRows` map stays
populated forever, still referenced by the task records in
`analysisTaskInfoMap` until they are individually evicted. On a busy cluster
with many auto-analyze job records this can still leave the large maps this PR
targets alive in the heap. Consider clearing the shared map when a job entry is
evicted (the `remove` in `replayCreateAnalysisJob`), or as part of
`analysisJobIdToTaskMap.remove(jobId)` in `updateTaskStatus`.
--
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]
