-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
vtorc
: use sqlite3 SQL dialect only
#17120
vtorc
: use sqlite3 SQL dialect only
#17120
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Tim Vaillancourt <[email protected]>
9086cb0
to
4d73aab
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17120 +/- ##
==========================================
- Coverage 67.17% 67.15% -0.03%
==========================================
Files 1571 1570 -1
Lines 252246 252184 -62
==========================================
- Hits 169458 169355 -103
- Misses 82788 82829 +41 ☔ View full report in Codecov by Sentry. |
insert or ignore into topology_recovery_steps ( | ||
recovery_step_id, recovery_id, audit_at, message | ||
) values ( | ||
?, ?, datetime('now'), ?, |
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.
?, ?, datetime('now'), ?, | |
?, ?, datetime('now'), ? |
I'd say it's a lot easier to review / test this and not do this in the same PR. Fixing the style is maybe something to separate out into a different change so it's essentially a no-op. Makes also reviewing the changes here a lot easier to review then. |
Closing in favour of clone PR #17124 that passes CI |
Description
As agreed in a Vitess-Slack discussion, this PR removes the regex-based, inline translation of SQL dialects in
vtorc
and uses the pure sqlite3 dialect instead. This is to avoid the high CPU penalty we notice when 100s of tablets are watchedThe majority of the changes are changing use of
now()
andunix_timestamp
to sqlite3 equivalents we were rewritting queries toWhile I made this switch I made
\t
white space and capitalisation of SQL consistent. It seems the majority of SQL was lower-case, so I standardised on this - happy to do the oppositeRelated Issue(s)
Checklist
Deployment Notes