-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-16237: Add execution info to logs about aborted queries #2172
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
base: main
Are you sure you want to change the base?
Conversation
Checklist before you submit for review
|
8bfa5fc to
ef4dfe6
Compare
|
ekaterinadimitrova2
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.
Looks good to me - test coverage, CI results, code-wise.
I would probably add some small notes to the observability/release notes section:
- how much the logs are growing with this addition - we made similar back of the envelope calculations in the other ticket, so good to mention
- how these logs can be used maybe?
- we need to list the new config properties
Good call on the alignment of the properties with Apache, I didn't notice those properties were pulled to CassandraRelevantProperties in the new branches.
Pending commit on the approval of the other PR which I am moving to review now too.
|
Thanks for the review. I have added release/operability/observability notes for this. The calculations about the disk space are the same as for slow but not aborted queries, since the messages are identical. I have added them for the notes anyway. Regarding new properties, we are not adding anything new nor renaming the existing |
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 guess we should merge the notes of all three tickets (16123, 15260 and 16237) into a single entry at release time.
I suggest we remove notes from the previous tickets and update them only in the last one.
Do you mean making here a combined note about both slow and aborted queries, even if this issue is only about aborted? |
ef4dfe6 to
0043ca5
Compare
I have updated the notes for this issue to be a combination of it's real changes, CNDB-15260 and CNDB-15260. I'll drop the notes in the merged issues, with some comment pointing to this issue, when I merge this one. |
|
Everything looks great on both PRs and the issue, thanks. My +1 stands |
|
It seems Sonar checks have failed for the second time, running once again. |
06d2429 to
0043ca5
Compare
❌ Build ds-cassandra-pr-gate/PR-2172 rejected by Butler2 regressions found Found 2 new test failures
Found 2 known test failures |



What is the issue
CNDB-15260 added SAI-specific execution information to the log reports produced for slow queries, and CNDB-15260 will soon add generic execution information for all non-SAI read commands. These details are produced only for queries that are reported as slow but successful by
MonitoringTask. However,MonitoringTaskalso produces log reports for queries that are slow enough to be aborted. We should probably also add execution information to the log reports for aborted queries.What does this PR fix and why was it fixed
Adds execution information to the log reports for aborted queries. The extension of the log messages is identical to what we did for slow successful queries in CNDB-15260 and CNDB-15260, but applied to aborted queries:
This also renames the
CassandraRelevantPropertiesrelated to monitoring, to align them with ASF/CC5. The renames are:SLOW_QUERY_LOG_MONITORING_REPORT_INTERVAL_IN_MS->MONITORING_REPORT_INTERVAL_MSSLOW_QUERY_LOG_MONITORING_MAX_OPERATIONS->MONITORING_MAX_OPERATIONSSLOW_QUERY_LOG_EXECUTION_INFO_ENABLED->MONITORING_EXECUTION_INFO_ENABLEDSAI_SLOW_QUERY_LOG_EXECUTION_INFO_ENABLED->SAI_MONITORING_EXECUTION_INFO_ENABLEDThis naming also makes more sense considering that monitoring is applied to both slow and aborted queries.
Please note that the real underlying system properties are unchanged, and this only renames their enum names.
All four enums are not present in any CC release yet.