Skip to content
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

vttablet sidecar schema:use schemadiff to reach desired schema on tablet init replacing the withDDL-based approach #11520

Merged
merged 15 commits into from
Feb 1, 2023

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Oct 17, 2022

Description

This PR changes the way we maintain the _vt schema. Instead of using withddl introduced in #6348 we use a declarative approach. This is made possible by schemadiff which is used in Online DDL's declarative strategy.

The desired schema is specified, one per table. A new module sidecardb, compares this to the existing schema and performs the required create or alter to reach it. This is done on the primary, on every vttablet startup.

An intended side-effect of this PR is to be able to minimize the time window, on a replica, where the database is NOT in super-read-only mode.

The sidecar tables local_metadata and shard_metadata are no longer in use and all references to them are removed in this PR. There were used previously for Orchestrator support, which has been superseded by vtorc

Notes

Replicas cannot be started with super-read-only on

We cannot start a replica with super-read-only on since we have a bootstrap problem: vttablet needs to connect to its mysql instance to manage it. For that it needs to user credentials. To create user credentials we first need to create the users. This is done by executing the queries in init_db.sql, when the database is first created.

Also until a primary is selected we don't know if the current vttablet is going to be a replica or not. If it becomes a primary then it will insert a row into reparent_journal for the other replicas to move forward as replicas.

Implementation Notes

Working Notes related to implementation, todos and misc notes, in no particular order, to be appropriately folded into the main description before completion.

go/vt/vttablet/tabletserver/schema/engine.go

The schema creation/updation is done in EnsureConnectionAndDB(), both for replicas and primary

config/init_db.sql

  • drop metadata tables
  • create orc_client_user

Incidental flaky test fixes

  • go/vt/servenv/servenv.go
  • go/vt/vtctl/grpcvtctldserver/server.go
  • go/vt/vtexplain/vtexplain_vtgate.go, go/vt/vtgate/querylog.go (QueryLogger)
  • go/vt/vtgate/vschemaacl/vschemaacl.go
  • TestFullStatus go/test/endtoend/reparent/plannedreparent/reparent_test.go

Implementation Todos

  • add comments to schema files
  • validate with existing tables in main _vt to confirm schema is current wrt datatypes
  • merge schemadiff changes and remove stripcharset
    DemotePrimary()
  • check if we still need to "create _vt if not exists" if _vt already exists, to get a binlog entry
  • check if twopc tables need to be in _vt schema
  • Upgrade/downgrade tests: do we need more or will current ones do?
  • Remove old logic. Currently we have left the code for withddl and health_streamer init conditional on the new flag
    • go/vt/vttablet/tabletserver/health_streamer.go
    • go/vt/withddl/withddl.go
    • online ddl: ApplyDDL should be empty (check for other such inits ...)
    • remove Create/Alter-ReparentJournal
  • Remove ALL withddls
  • Change dummy database creation back to _vt, during first tablet load
  • Remove init-vt-schema-on-tablet-init flag and make new schema init to be the only way
  • Get all tests that skipped the flag to work with new schema init
  • unit tests for sidecardb
  • deprecate (not remove) init metadata flag

Minor:

  • Remove vtbackup changes for readonly

Related Issue(s)

#10133

Previous related PRs

These were created previously and closed since those didn't work well.
#11235

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Follow-up todos after the initial merge

Docs

  • update RFC to match actual implementation
  • release notes
  • design docs on website
    • how vttablet inits
    • how Vitess cluster upgrade works, wrt _vt schema updats (update/extend the Life of a cluster doc)
    • Developer guide/how-to make changes to _vt schema
  • Review/fix flow of DemotePrimary
  • Manual tests, possibly add to sidecar e2e tests to test different flows
  • add hint to schemadiff to add database name and remove useDB from Exec
  • figure out why e2e sidecardb test is failing after multiple PRSes. Possibly related to incorrect code being run as part of

@rohit-nayak-ps rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: General Changes throughout the code base Do Not Merge labels Oct 17, 2022
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Oct 17, 2022

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

@rohit-nayak-ps rohit-nayak-ps added the Skip CI Skip CI actions from running label Oct 18, 2022
@frouioui frouioui added the Early in Cycle These items are prioritised for the current release cycle label Nov 18, 2022
@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch from 0bd5572 to 2bbae28 Compare December 6, 2022 16:24
@rohit-nayak-ps rohit-nayak-ps removed the Skip CI Skip CI actions from running label Dec 6, 2022
@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch 2 times, most recently from 7cd38bd to 27a206f Compare December 8, 2022 18:15
@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch 6 times, most recently from d33a2f9 to a761961 Compare December 17, 2022 10:17
@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch 2 times, most recently from ba61177 to cd581e0 Compare December 26, 2022 11:05
@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch 3 times, most recently from 348d0b1 to dd7865d Compare January 7, 2023 22:37
@rohit-nayak-ps rohit-nayak-ps changed the title WIP: Tablet init refactor: change way _vt schema is created/updated, reduce duration when replicas are not super-read-only WIP: Tablet init refactor: use schemadiff to reach desired _vt schema on tablet init instead of the withDDL-based approach Jan 9, 2023
@rohit-nayak-ps rohit-nayak-ps changed the title WIP: Tablet init refactor: use schemadiff to reach desired _vt schema on tablet init instead of the withDDL-based approach WIP: _vt schema:use schemadiff to reach desired _vt schema on tablet init instead of the withDDL-based approach Jan 9, 2023
@rohit-nayak-ps rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch 3 times, most recently from 1d2fef9 to ca49239 Compare January 11, 2023 13:03
@@ -53,44 +52,6 @@ var (
`
)

func TestBlockedLoadKeyspace(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is no longer valid after the change in how we initialize _vt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a nice bonus, we no longer have to worry about this test being flaky (which it has been in the past).

@rohit-nayak-ps rohit-nayak-ps merged commit 4f5ab22 into vitessio:main Feb 1, 2023

CREATE TABLE IF NOT EXISTS _vt.vdiff
(
`id` bigint(20) NOT NULL AUTO_INCREMENT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohit-nayak-ps For a separate follow up PR, but I think we should drop lengths for bigint and other int types, since that is deprecated and on MySQL 8.0 doesn't do anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 and also worth mentioning that schemadiff ignores the length values (unless it's 1), so whether they exist or not, does not make any change to prod.

@shlomi-noach shlomi-noach deleted the rn-tablet-init-refactor branch February 7, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: General Changes throughout the code base Do Not Merge Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants