-
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
[vtadmin-api] Add /vtexplain endpoint #7528
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,21 +18,27 @@ package vtadmin | |
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"net/http" | ||
"strings" | ||
"sync" | ||
|
||
"github.com/gorilla/handlers" | ||
"github.com/gorilla/mux" | ||
|
||
"vitess.io/vitess/go/trace" | ||
"vitess.io/vitess/go/vt/concurrency" | ||
"vitess.io/vitess/go/vt/topo" | ||
"vitess.io/vitess/go/vt/vtadmin/cluster" | ||
"vitess.io/vitess/go/vt/vtadmin/grpcserver" | ||
vtadminhttp "vitess.io/vitess/go/vt/vtadmin/http" | ||
vthandlers "vitess.io/vitess/go/vt/vtadmin/http/handlers" | ||
"vitess.io/vitess/go/vt/vtadmin/sort" | ||
"vitess.io/vitess/go/vt/vterrors" | ||
"vitess.io/vitess/go/vt/vtexplain" | ||
|
||
topodatapb "vitess.io/vitess/go/vt/proto/topodata" | ||
vtadminpb "vitess.io/vitess/go/vt/proto/vtadmin" | ||
vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata" | ||
vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" | ||
|
@@ -83,6 +89,7 @@ func NewAPI(clusters []*cluster.Cluster, opts grpcserver.Options, httpOpts vtadm | |
router.HandleFunc("/schemas", httpAPI.Adapt(vtadminhttp.GetSchemas)).Name("API.GetSchemas") | ||
router.HandleFunc("/tablets", httpAPI.Adapt(vtadminhttp.GetTablets)).Name("API.GetTablets") | ||
router.HandleFunc("/tablet/{tablet}", httpAPI.Adapt(vtadminhttp.GetTablet)).Name("API.GetTablet") | ||
router.HandleFunc("/vtexplain", httpAPI.Adapt(vtadminhttp.VTExplain)).Name("API.VTExplain") | ||
|
||
// Middlewares are executed in order of addition. Our ordering (all | ||
// middlewares being optional) is: | ||
|
@@ -534,6 +541,47 @@ func (api *API) getTablets(ctx context.Context, c *cluster.Cluster) ([]*vtadminp | |
return ParseTablets(rows, c) | ||
} | ||
|
||
// findTablet returns the first tablet in a given cluster that satisfies the filter function. | ||
func (api *API) findTablet(ctx context.Context, cluster *cluster.Cluster, filter func(*vtadminpb.Tablet) bool) (*vtadminpb.Tablet, error) { | ||
tablets, err := api.findTablets(ctx, cluster, filter, 1) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if len(tablets) != 1 { | ||
return nil, ErrNoTablet | ||
} | ||
|
||
return tablets[0], nil | ||
} | ||
|
||
// findTablets returns the first N tablets in the given cluster that satisfy | ||
// the filter function. If N = -1, then all matching tablets are returned. | ||
// Ordering is not guaranteed, and callers should write their filter functions accordingly. | ||
func (api *API) findTablets(ctx context.Context, cluster *cluster.Cluster, filter func(*vtadminpb.Tablet) bool, n int) ([]*vtadminpb.Tablet, error) { | ||
tablets, err := api.getTablets(ctx, cluster) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if n == -1 { | ||
n = len(tablets) | ||
} | ||
|
||
results := make([]*vtadminpb.Tablet, 0, n) | ||
for _, t := range tablets { | ||
if len(results) >= n { | ||
break | ||
} | ||
|
||
if filter(t) { | ||
results = append(results, t) | ||
} | ||
} | ||
|
||
return results, nil | ||
} | ||
|
||
func (api *API) getClustersForRequest(ids []string) ([]*cluster.Cluster, []string) { | ||
if len(ids) == 0 { | ||
clusterIDs := make([]string, 0, len(api.clusters)) | ||
|
@@ -555,3 +603,152 @@ func (api *API) getClustersForRequest(ids []string) ([]*cluster.Cluster, []strin | |
|
||
return clusters, ids | ||
} | ||
|
||
// VTExplain is part of the vtadminpb.VTAdminServer interface. | ||
func (api *API) VTExplain(ctx context.Context, req *vtadminpb.VTExplainRequest) (*vtadminpb.VTExplainResponse, error) { | ||
span, ctx := trace.NewSpan(ctx, "API.VTExplain") | ||
defer span.Finish() | ||
|
||
if req.Cluster == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This parameter validation feels a bit clumsy so I'm open to suggestions. Since, in the HTTP handler, these parameters are GET params (i..e, Also, the I briefly considered breaking the goroutines out into separate functions, but frankly it made it harder to read + I don't expect the functions have any reusability, since VTExplain is the only instance requiring all this string marshalling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It would be really cool if there was a good way to split out a mux route into ${parameter validation} + ${logic} phases, where, where the params extracted/parsed/validated by phase 1 are available in the context (not necessarily the literal Go context, but also not necessarily not the literal Go context) of phase 2, but I don't see that, or something like it, from a quick skim of the mux docs. Maybe we should keep thinking about this!
Agreed that would potentially make things harder to understand, and better to have the longer, but cohesive single function.
we can also ....................... add a more typed interface to vtexplain :llama_hands: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, something along these lines would be useful. We should, at some point, revisit the HTTP response envelopes (particularly for errors) that we're using internally. I think there are ways we can improve on what we have. Or.. we could try gRPC web again, lol.
I really want this. The string marshalling is easy to mess up + is a bit tedious to debug. |
||
return nil, fmt.Errorf("%w: cluster ID is required", ErrInvalidRequest) | ||
} | ||
|
||
if req.Keyspace == "" { | ||
return nil, fmt.Errorf("%w: keyspace name is required", ErrInvalidRequest) | ||
} | ||
|
||
if req.Sql == "" { | ||
return nil, fmt.Errorf("%w: SQL query is required", ErrInvalidRequest) | ||
} | ||
|
||
c, ok := api.clusterMap[req.Cluster] | ||
if !ok { | ||
return nil, ErrUnsupportedCluster | ||
} | ||
|
||
tablet, err := api.findTablet(ctx, c, func(t *vtadminpb.Tablet) bool { | ||
return t.Tablet.Keyspace == req.Keyspace && topo.IsInServingGraph(t.Tablet.Type) && t.Tablet.Type != topodatapb.TabletType_MASTER && t.State == vtadminpb.Tablet_SERVING | ||
}) | ||
|
||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if err := c.Vtctld.Dial(ctx); err != nil { | ||
return nil, err | ||
} | ||
|
||
var ( | ||
wg sync.WaitGroup | ||
er concurrency.AllErrorRecorder | ||
|
||
// Writes to these three variables are, in the strictest sense, unsafe. | ||
// However, there is one goroutine responsible for writing each of these | ||
// values (so, no concurrent writes), and reads are blocked on the call to | ||
// wg.Wait(), so we guarantee that all writes have finished before attempting | ||
// to read anything. | ||
srvVSchema string | ||
schema string | ||
shardMap string | ||
) | ||
|
||
wg.Add(3) | ||
|
||
// GetSchema | ||
go func(c *cluster.Cluster) { | ||
defer wg.Done() | ||
|
||
res, err := c.Vtctld.GetSchema(ctx, &vtctldatapb.GetSchemaRequest{ | ||
TabletAlias: tablet.Tablet.Alias, | ||
}) | ||
|
||
if err != nil { | ||
er.RecordError(err) | ||
return | ||
} | ||
|
||
schemas := make([]string, len(res.Schema.TableDefinitions)) | ||
for i, td := range res.Schema.TableDefinitions { | ||
schemas[i] = td.Schema | ||
} | ||
|
||
schema = strings.Join(schemas, ";") | ||
}(c) | ||
|
||
// GetSrvVSchema | ||
go func(c *cluster.Cluster) { | ||
defer wg.Done() | ||
|
||
res, err := c.Vtctld.GetSrvVSchema(ctx, &vtctldatapb.GetSrvVSchemaRequest{ | ||
Cell: tablet.Tablet.Alias.Cell, | ||
}) | ||
|
||
if err != nil { | ||
er.RecordError(err) | ||
return | ||
} | ||
|
||
ksvs, ok := res.SrvVSchema.Keyspaces[req.Keyspace] | ||
if !ok { | ||
er.RecordError(fmt.Errorf("%w: keyspace %s", ErrNoSrvVSchema, req.Keyspace)) | ||
return | ||
} | ||
|
||
ksvsb, err := json.Marshal(&ksvs) | ||
if err != nil { | ||
er.RecordError(err) | ||
return | ||
} | ||
|
||
srvVSchema = fmt.Sprintf(`{"%s": %s}`, req.Keyspace, string(ksvsb)) | ||
}(c) | ||
|
||
// FindAllShardsInKeyspace | ||
go func(c *cluster.Cluster) { | ||
defer wg.Done() | ||
|
||
ksm, err := c.Vtctld.FindAllShardsInKeyspace(ctx, &vtctldatapb.FindAllShardsInKeyspaceRequest{ | ||
Keyspace: req.Keyspace, | ||
}) | ||
|
||
if err != nil { | ||
er.RecordError(err) | ||
return | ||
} | ||
|
||
vtsm := make(map[string]*topodatapb.Shard) | ||
for _, s := range ksm.Shards { | ||
vtsm[s.Name] = s.Shard | ||
} | ||
|
||
vtsb, err := json.Marshal(&vtsm) | ||
if err != nil { | ||
er.RecordError(err) | ||
return | ||
} | ||
|
||
shardMap = fmt.Sprintf(`{"%s": %s}`, req.Keyspace, string(vtsb)) | ||
}(c) | ||
|
||
wg.Wait() | ||
|
||
if er.HasErrors() { | ||
return nil, er.Error() | ||
} | ||
|
||
opts := &vtexplain.Options{ReplicationMode: "ROW"} | ||
|
||
if err := vtexplain.Init(srvVSchema, schema, shardMap, opts); err != nil { | ||
return nil, err | ||
} | ||
|
||
plans, err := vtexplain.Run(req.Sql) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
response := vtexplain.ExplainsAsText(plans) | ||
return &vtadminpb.VTExplainResponse{ | ||
Response: response, | ||
}, nil | ||
} |
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.
Maybe these functions make more sense on the
cluster.Cluster
type? That would at least help break this file up a bit. Totally up to you!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 would definitely like to move them elsewhere. I think, though, that moving them to
cluster.Cluster
might introduce a weird dependency.Specifically, per your suggestion, I want to move the following three functions out of vtadmin/api.go and into vtadmin/cluster/cluster.go:
However,
getTablets
callsParseTablets
, which is in vtadmin/tablets.go (package vtadmin
), which would (I think) cause a circular dependency. I assume this is undesirable in golang, right?I have two ideas, not sure which (if either) make sense to do:
package tablet
vtadmin/tablet/tablet.go
(analogous tovtadmin/cluster/cluster.go
)vtadmin/tablets.go
, since the other functions intablets.go
also take a cluster argument. It seems like not a bad fit.Happy to take any suggestion you have!
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.
Yeah, I think anything that is like
func (api *API) DoThingWithTablets(ctx context.Context, c *cluster.Cluster)
we should try (it might have some other dependencies onapi
, which could get uhhhh interesting) to make it afunc (c *Cluster) DoThingWithTablets(ctx context.Context)
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.
@ajm188 hmmm ok. In that case, I will have to move
tablet.go
(and itsParseTablet
function) into its ownpackage tablet
socluster.go
can use it without adding a (circular) dependency onpackage vtadmin
, right? It seems reasonable to me, I just want to double check before I move things around.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.
Yeah, that's what I was saying with where it should be
func (c *Cluster) ParseTablets(ctx context.Context, ...) ([]*vtadminpb.Tablet, error)
instead offunc (api *API) ParseTablets(ctx context.Context, c *cluster.Cluster, ...) (...)
.Also, I'm not super convinced that needs to remain a public function. It doesn't really feel like part of the API of a cluster, while "find N tablets in the cluster" definitely does.
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.
@ajm188 that sounds really good.
Moving things around ends up being a rather big + messy commit, and it'd be nice to merge this one in since I'm branched off of it for front-end reasons.
Would it be ok with you if I moved everything into
clusters.go
in a separate PR? I can do it today, right after this, and add a bunch of tests as penance.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.
Yeah, let's file an issue for that, because I'll forget, but sg2m to merge