[refactor](status) Refactor status handling in agent task#11940
[refactor](status) Refactor status handling in agent task#11940yiguolei merged 4 commits intoapache:masterfrom
Conversation
platoneko
force-pushed
the
refactor_status_handling_task
branch
3 times, most recently
from
d029815 to
66f2c91
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Please rebase to fix the P0
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
LGTM.
except maybe add redundant include.
|
PR approved by anyone and no changes requested. |
platoneko
force-pushed
the
refactor_status_handling_task
branch
from
66f2c91 to
91d2e57
Compare
| for (const auto& rs : consistent_rowsets) { | ||
| status = rs->copy_files_to(full_path, rs->rowset_id()); | ||
| if (!status.ok()) { | ||
| Status ret = FileUtils::remove_all(full_path); |
There was a problem hiding this comment.
This line is removed, then there exists garbage in this folder?
There was a problem hiding this comment.
EngineStorageMigrationTask::_migrate will do this garbage collection.
| } | ||
| if (!build_all_report_tablets_info_status.ok()) { | ||
| LOG(WARNING) << "build all report tablets info failed. status: " | ||
| << build_all_report_tablets_info_status; |
There was a problem hiding this comment.
If build failed, some logic in handle report maybe wrong, it think this logic should not be removed.
There was a problem hiding this comment.
In fact, build_all_report_tablets_info always returns OK, I even consider let build_all_report_tablets_info return void.
| } | ||
|
|
||
| LOG(INFO) << "finish to process delete data. res=" << res; | ||
| return res; |
There was a problem hiding this comment.
All these tasks have a same problem. If the task is success, there is no log. So if a table lost some data or data is wrong, we could not find what happened on this tablet. I think we should print some success log to help us debug. Maybe could add a log in EngineTask.
There was a problem hiding this comment.
Such log will be print in _push_worker_thread_callback:
if (status.ok()) {
LOG_INFO("successfully execute push task")
.tag("signature", agent_task_req.signature)
.tag("tablet_id", push_req.tablet_id)
.tag("push_type", push_req.push_type);
++_s_report_version;
finish_task_request.__set_finish_tablet_infos(tablet_infos);
} else {
LOG_WARNING("failed to execute push task")
.tag("signature", agent_task_req.signature)
.tag("tablet_id", push_req.tablet_id)
.tag("push_type", push_req.push_type)
.error(status);
}
Refactor TaggableLogger Refactor status handling in agent task: Unify log format in TaskWorkerPool Pass Status to the top caller, and replace some OLAPInternalError with more detailed error message Status Premature return with the opposite condition to reduce indention
Refactor TaggableLogger Refactor status handling in agent task: Unify log format in TaskWorkerPool Pass Status to the top caller, and replace some OLAPInternalError with more detailed error message Status Premature return with the opposite condition to reduce indention
Refactor TaggableLogger Refactor status handling in agent task: Unify log format in TaskWorkerPool Pass Status to the top caller, and replace some OLAPInternalError with more detailed error message Status Premature return with the opposite condition to reduce indention
3 participants
Proposed changes
Issue Number: #11874
Problem summary
TaggableLoggerStatusto the top caller, and replace someOLAPInternalErrorwith more detailed error messageStatusChecklist(Required)
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...