-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Improved failure messages for MSQCompactionRunner. #18787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Include the first failure message in the task status itself, so it is not necessary to fetch task logs to see the error message. Also, don't log the entire task JSON for failed subtasks. It is logged once when a subtask is initially run, and that's enough.
kfaraz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| final TaskStatus eachResult = eachTask.run(toolbox); | ||
| if (!eachResult.isSuccess()) { | ||
| if (task.isReady(toolbox.getTaskActionClient())) { | ||
| log.info("Running MSQControllerTask[%d]: %s", taskCnt, json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It seems kind of weird that the MSQControllerTask[%d] is not the the task id but the taskcnt.
Should be rename this to something like
"Running MSQCompaction task number[%d] with payload : %s"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated them all to be like MSQControllerTask number[%d].
| if (!taskStatus.isSuccess()) { | ||
| failCnt++; | ||
| log.warn("Failed to run MSQControllerTask: [%s].\nTrying the next MSQControllerTask.", json); | ||
| log.warn("Failed to run MSQControllerTask[%d]: %s", taskCnt, taskStatus.getErrorMsg()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar task number comment here.
Include the first failure message in the task status itself, so it is not necessary to fetch task logs to see the error message.
Also, don't log the entire task JSON for failed subtasks. It is logged once when a subtask is initially run, and that's enough.