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

[vtctld] Migrate GetSchema #7346

Merged
merged 5 commits into from
Jan 22, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Jan 22, 2021

Description

This migrates the GetSchema vtctl command to a vtctld rpc, and reimplements the old command to use the new RPC under the hood.

Example:

❯ vtctldclient --server "localhost:15999" GetSchema zone1-0000000100
{
  "database_schema": "CREATE DATABASE /*!32312 IF NOT EXISTS*/ {{.DatabaseName}} /*!40100 DEFAULT CHARACTER SET utf8 */",
  "table_definitions": [
    {
      "name": "corder",
      "schema": "CREATE TABLE `corder` (\n  `order_id` bigint(20) NOT NULL AUTO_INCREMENT,\n  `customer_id` bigint(20) DEFAULT NULL,\n  `sku` varbinary(128) DEFAULT NULL,\n  `price` bigint(20) DEFAULT NULL,\n  PRIMARY KEY (`order_id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8",
      "columns": [
        "order_id",
        "customer_id",
        "sku",
        "price"
      ],
      "primary_key_columns": [
        "order_id"
      ],
      "type": "BASE TABLE",
      "data_length": "16384",
      "row_count": "0",
      "fields": [
        {
          "name": "order_id",
          "type": "INT64",
          "table": "corder",
          "org_table": "corder",
          "database": "vt_commerce",
          "org_name": "order_id",
          "column_length": 20,
          "charset": 63,
          "decimals": 0,
          "flags": 49667,
          "column_type": ""
        },
        {
          "name": "customer_id",
          "type": "INT64",
          "table": "corder",
          "org_table": "corder",
          "database": "vt_commerce",
          "org_name": "customer_id",
          "column_length": 20,
          "charset": 63,
          "decimals": 0,
          "flags": 32768,
          "column_type": ""
        },
        {
          "name": "sku",
          "type": "VARBINARY",
          "table": "corder",
          "org_table": "corder",
          "database": "vt_commerce",
          "org_name": "sku",
          "column_length": 128,
          "charset": 63,
          "decimals": 0,
          "flags": 128,
          "column_type": ""
        },
        {
          "name": "price",
          "type": "INT64",
          "table": "corder",
          "org_table": "corder",
          "database": "vt_commerce",
          "org_name": "price",
          "column_length": 20,
          "charset": 63,
          "decimals": 0,
          "flags": 32768,
          "column_type": ""
        }
      ]
    },
    {
      "name": "customer",
      "schema": "CREATE TABLE `customer` (\n  `customer_id` bigint(20) NOT NULL AUTO_INCREMENT,\n  `email` varbinary(128) DEFAULT NULL,\n  PRIMARY KEY (`customer_id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8",
      "columns": [
        "customer_id",
        "email"
      ],
      "primary_key_columns": [
        "customer_id"
      ],
      "type": "BASE TABLE",
      "data_length": "16384",
      "row_count": "0",
      "fields": [
        {
          "name": "customer_id",
          "type": "INT64",
          "table": "customer",
          "org_table": "customer",
          "database": "vt_commerce",
          "org_name": "customer_id",
          "column_length": 20,
          "charset": 63,
          "decimals": 0,
          "flags": 49667,
          "column_type": ""
        },
        {
          "name": "email",
          "type": "VARBINARY",
          "table": "customer",
          "org_table": "customer",
          "database": "vt_commerce",
          "org_name": "email",
          "column_length": 128,
          "charset": 63,
          "decimals": 0,
          "flags": 128,
          "column_type": ""
        }
      ]
    },
    {
      "name": "product",
      "schema": "CREATE TABLE `product` (\n  `sku` varbinary(128) NOT NULL,\n  `description` varbinary(128) DEFAULT NULL,\n  `price` bigint(20) DEFAULT NULL,\n  PRIMARY KEY (`sku`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8",
      "columns": [
        "sku",
        "description",
        "price"
      ],
      "primary_key_columns": [
        "sku"
      ],
      "type": "BASE TABLE",
      "data_length": "16384",
      "row_count": "0",
      "fields": [
        {
          "name": "sku",
          "type": "VARBINARY",
          "table": "product",
          "org_table": "product",
          "database": "vt_commerce",
          "org_name": "sku",
          "column_length": 128,
          "charset": 63,
          "decimals": 0,
          "flags": 20611,
          "column_type": ""
        },
        {
          "name": "description",
          "type": "VARBINARY",
          "table": "product",
          "org_table": "product",
          "database": "vt_commerce",
          "org_name": "description",
          "column_length": 128,
          "charset": 63,
          "decimals": 0,
          "flags": 128,
          "column_type": ""
        },
        {
          "name": "price",
          "type": "INT64",
          "table": "product",
          "org_table": "product",
          "database": "vt_commerce",
          "org_name": "price",
          "column_length": 20,
          "charset": 63,
          "decimals": 0,
          "flags": 32768,
          "column_type": ""
        }
      ]
    }
  ],
  "version": "4a696261cc73c605a887ad6a547dce18"
}
❯ vtctldclient --server "localhost:15999" GetSchema -n zone1-0000000100
corder
customer
product

Related Issue(s)

Checklist

  • Should this PR be backported? No
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build
  • VTAdmin

@ajm188 ajm188 requested a review from doeg January 22, 2021 13:40
@ajm188 ajm188 requested review from deepthi and sougou as code owners January 22, 2021 13:40
@ajm188 ajm188 force-pushed the am_vtctld_getschema branch from 4956ab5 to da1437c Compare January 22, 2021 15:30
@ajm188
Copy link
Contributor Author

ajm188 commented Jan 22, 2021

👋 I'm going to be pushing some commits to try to debug the plugin registration problem, apologies for the noise. I'll ping when I've fixed the issue.

@ajm188 ajm188 force-pushed the am_vtctld_getschema branch from 7a9444c to 3ee391b Compare January 22, 2021 16:36
@ajm188
Copy link
Contributor Author

ajm188 commented Jan 22, 2021

Okay @doeg this should be good to go now! Sorry about that

Copy link
Contributor

@doeg doeg left a comment

Choose a reason for hiding this comment

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

This looks so good and a million thank yous again for doing this. 🐶 ✨ I can't wait to add VTExplain!

The implementation looks great. I just have a question about a couple test cases. :')

shouldErr: false,
},
{
name: "table names take precedence over table sizes",
Copy link
Contributor

Choose a reason for hiding this comment

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

Small question to double check my read of this. 😊

The command/schema.go implementation throws an error if both are specified. For what it's worth, explicitly erroring when both flags are set seems preferable to (since it's less surprising than) implicit precedence... and I have a feeling that's why you wrote command/schema.go to error, right?

And I have a feeling that we can't return an error here, and instead have precedence behaviour, because legacy reasons?

To be clear, I think the discrepancy between the two is fine, and what you have is great. I just want to make sure I follow the difference. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this isn't about maintaining "legacy" compatibility, but more that the options are fundamentally incompatible with each other. If I could have made the parser itself enforce the mutual exclusion, that would have been my highest preference.

I think there's an argument to be made that the server implementation itself should error in that case (instead of just relying on the CLI to catch it), but I think I would disagree on "be permissive in what you accept" kinds of reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense! I think the behaviour as you've written it is really well documented, between the protobuf comments + tests, so "surprising" was probably the wrong word. 😆 Thank you for explaining your thoughts; they are always good. :)

}
}

tests := []*struct {
Copy link
Contributor

@doeg doeg Jan 22, 2021

Choose a reason for hiding this comment

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

(Bah, GitHub didn't save this question + it's the only blocking question I had.)

🙈 I'm sorry to be the bearer of this question, but did you consider test cases for the tables and/or exclude_tables parameters?

I'm also wondering if it might be advantageous to mock > 1 tables for these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No because those are passed straight through to the tabletmanagerclient, so testing is that is their problem, imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or I guess, "yes, I considered it; here's why I'm not" to actually answer your question 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh I missed that! That makes sense, thanks.

Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

lgtm
things are coming together nicely :-)

@rohit-nayak-ps rohit-nayak-ps merged commit e34764d into vitessio:master Jan 22, 2021
@askdba askdba added this to the v9.0 milestone Jan 25, 2021
ajm188 pushed a commit to tinyspeck/vitess that referenced this pull request Feb 11, 2021
setassociative pushed a commit to tinyspeck/vitess that referenced this pull request Mar 11, 2021
[vtctld] Migrate GetSchema

Signed-off-by: Richard Bailey <[email protected]>
ajm188 pushed a commit to tinyspeck/vitess that referenced this pull request Apr 6, 2021
[vtctld] Migrate GetSchema

Signed-off-by: Andrew Mason <[email protected]>
@ajm188 ajm188 deleted the am_vtctld_getschema branch May 29, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate simple read-only schema commands to vtctld gRPC API
4 participants