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

[vtctldserver] Migrate remaining ServingGraph rpcs to VtctldServer #8249

Merged
merged 4 commits into from
Aug 30, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Jun 2, 2021

Description

This PR migrates the following RPCs to the VtctldServer:

  • DeleteSrvVSchema
  • GetSrvKeyspaceNames
Usage

Everything on the local example.

❯ vtctldclient --server "localhost:15999" GetSrvKeyspaceNames
{
  "zone1": {
    "names": [
      "commerce"
    ]
  }
}

After running through the 2xx examples:

❯ vtctldclient --server "localhost:15999" GetSrvKeyspaceNames
{
  "zone1": {
    "names": [
      "commerce",
      "customer"
    ]
  }
}
❯ vtctldclient --server "localhost:15999" GetSrvVSchemas
{
  "zone1": {
    "keyspaces": {
      "commerce": {
        "tables": {
          "product": {}
        }
      },
      "customer": {
        "tables": {
          "corder": {},
          "customer": {}
        }
      }
    },
    "routing_rules": {}
  }
}
❯ vtctldclient --server "localhost:15999" DeleteSrvVSchema zone1
❯ vtctlclient -server "localhost:15999" TopoCat -cell zone1 -decode_proto_json SrvVSchema
TopoCat: Get(SrvVSchema) failed: node doesn't exist: /vitess/zone1/SrvVSchema
null
TopoCat Error: rpc error: code = Unknown desc = TopoCat: some paths had errors
E0602 17:39:16.972427   81449 main.go:72] remote error: rpc error: code = Unknown desc = TopoCat: some paths had errors

Related Issue(s)

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@@ -647,6 +663,41 @@ func (s *VtctldServer) GetShard(ctx context.Context, req *vtctldatapb.GetShardRe
}, nil
}

// GetSrvKeyspaceNames is part of the vtctlservicepb.VtctldServer interface.
func (s *VtctldServer) GetSrvKeyspaceNames(ctx context.Context, req *vtctldatapb.GetSrvKeyspaceNamesRequest) (*vtctldatapb.GetSrvKeyspaceNamesResponse, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This implementation seems to diverge from the existent one in vtctl: https://github.com/tinyspeck/vitess/blob/22bdd8a9304ec2499414a2526ea644a2925259d5/go/vt/vtctl/vtctl.go#L3240

Curious of the rationale for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In VTAdmin we've been finding that we almost always want to get "all of the things" for a given resource, so this let's us make one call to a vtctld rather than one call to get all the cells and then one call per cell.

Which reminds me ... I meant to update the old implementation to call this one!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to demonstrate the old commands still work after my latest commit:

❯ vtctlclient -server "localhost:15999" GetSrvKeyspaceNames zone1
commerce

@ajm188 ajm188 force-pushed the am_vtctldserver_servinggraph_rpcs branch from 9f81489 to 4474886 Compare June 8, 2021 15:01
@ajm188
Copy link
Contributor Author

ajm188 commented Jun 8, 2021

rebased and regenerated protos

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 good to me, and the new GetSrvKeyspaceNames behaviour seems like a helpful default. You might want another +1 from @rafael or @deepthi, since this is ✨ new ✨

@ajm188 ajm188 force-pushed the am_vtctldserver_servinggraph_rpcs branch from 4474886 to 6548ad5 Compare August 26, 2021 23:51
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi deepthi merged commit d348b8a into vitessio:main Aug 30, 2021
@ajm188 ajm188 deleted the am_vtctldserver_servinggraph_rpcs branch August 30, 2021 20:13
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 remaining ServingGraph commands to VtctldServer
5 participants