- Notifications
You must be signed in to change notification settings - Fork857
Forward port for: print on violated block ordering (#21743)#21773
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:devel
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
* Added printing for violation of shutdown ordering* Exclude GatherNodes from order violation checks. Added more informative printing* Applied review comment. Also added ALERT. Also print if we are still starting async tasks although we already stopped them---------Co-authored-by: Max Neunhoeffer <max@arangodb.com>
| bool hasStoppedAsyncTasks = block->hasStoppedAsyncTasks(); | ||
| if (hasStoppedAsyncTasks) { | ||
| LOG_TOPIC("14d22", WARN, Logger::AQL) | ||
| << "[query#" << block->getQuery().id() << "] ALERT" | ||
| << block->printBlockInfo() | ||
| << " was asked to stop async task. We still start one. " | ||
| "This is an allowed rare race."; | ||
| } | ||
| auto stopGuard = | ||
| ScopeGuard([block, hasStoppedAsyncTasks]() noexcept { | ||
| if (hasStoppedAsyncTasks) { | ||
| LOG_TOPIC("14d21", WARN, Logger::AQL) | ||
| << "[query#" << block->getQuery().id() | ||
| << "] CLEAR ALERT" << block->printBlockInfo() | ||
| << " We completed the task of the aforementioned " | ||
| "race. All is fine."; | ||
| } | ||
| }); |
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.
These should not be onWARN, but probably onDEBUG.
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.
If they are DEBUG, then we do not see them in production, or?
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.
Unless we increase the log level on deployments of interest. But these aren't messages we generally want to see on WARN, or am I wrong?
| LOG_TOPIC("a6c2b", WARN, Logger::AQL) | ||
| << "ALERT Stopping async tasks for " << block->printBlockInfo() | ||
| << " but have already stopped dependency " | ||
| << seenDependency->printBlockInfo(); |
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.
I think this could actually be bumped to ERROR.
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.
Fine with me.
Scope & Purpose
This PR adds more visibility into a violated assumptions. The cleanup of AQL expects Blocks to be in a particular order in the cleanup lists. This PR should add visibility in cases where it is unexpectedly not.
Checklist
Related Information
(Please reference tickets / specification / other PRs etc)