Skip to content
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
4edafc4
Initial generic backfill cache implementation for vtadmin
Apr 20, 2022
b341819
disable structcheck for vtadmin/cache because it does not support gen…
Apr 20, 2022
d804752
Fix expiration semantics
Apr 20, 2022
1db42c0
cache configs
Apr 20, 2022
9dfca40
Stop taking bare strings in our cache layer
Apr 21, 2022
63f4701
let's give this a whirl
Apr 21, 2022
412702b
log so we know we are using the cache
Apr 21, 2022
c166e83
refactor toward supporting GetSchemas
Apr 21, 2022
91f7380
refactor, support GetSchemas, add tracing annotations
Apr 21, 2022
e92f40c
switch away from json.marshal, add log back
Apr 21, 2022
ae6d213
hey dummy what if you actually filled the cache
Apr 21, 2022
9834c88
fix logic for determining when we need an additional backfill
Apr 21, 2022
8aaee63
method extraction
Apr 21, 2022
c645184
the Final Refactor :chefs-kiss:
Apr 22, 2022
8f22f9d
add some tests
Apr 22, 2022
3458fe2
Add license headers, and some docs
Apr 22, 2022
5078086
add config hook point
Apr 22, 2022
f16c9b3
start testing backfill logic -- i found a bug!! yay :lolsob:
Apr 23, 2022
b3bb564
use unbuffered channel so we don't need a (potentially flaky) sleep
Apr 23, 2022
b5187dc
more tests
Apr 23, 2022
9550a8e
Install mechanism to propagate cache-refresh requests
Apr 24, 2022
ff1b5d8
add Debug for caches
Apr 25, 2022
41dd470
Fix LoadOne to correctly handle not-found
Apr 25, 2022
c2ad8a2
fix some data races
Apr 25, 2022
fe36b4b
Actually use the config
Apr 25, 2022
06b733f
more clones, also proto.Clone has weird 'zero value' vs nil behavior
Apr 25, 2022
384b33f
this test does not work if the cache has values
Apr 25, 2022
3e0642e
punt on fixing api.TestGetSchemas
Apr 26, 2022
c0a64f3
Fix test setup for FindSchema to support backfill
Apr 26, 2022
4990944
add io.Closer impls for api.API and cluster.Cluster
Apr 26, 2022
1904198
finally, at long last, fix the racy tests for reals
Apr 26, 2022
9d0c233
close clusters concurrently
Apr 27, 2022
df1701f
Just use a map to track lastFill times
May 5, 2022
ece1865
ctx/cancel->time.After
May 5, 2022
4e3fe30
Simpler Debug impl
May 5, 2022
b25708d
Remove unnecessary second Done() check
May 5, 2022
f1abecb
Merge branch 'vmain' into andrew/vtadmin/schema-cache
May 21, 2022
48f8d0a
PR feedback: reduce contention + add comments
May 21, 2022
a9d79c2
Merge branch 'vmain' into andrew/vtadmin/schema-cache
May 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ issues:
- errcheck
- goimports

- path: '^go/vt/vtadmin/cache/'
linters:
- structcheck

### BEGIN: errcheck exclusion rules. Each rule should be considered
# a TODO for removal after adding error checks to that package/file/etc,
# except where otherwise noted.
Expand Down
14 changes: 14 additions & 0 deletions go/cmd/vtadmin/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"vitess.io/vitess/go/trace"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/vtadmin"
"vitess.io/vitess/go/vt/vtadmin/cache"
"vitess.io/vitess/go/vt/vtadmin/cluster"
"vitess.io/vitess/go/vt/vtadmin/grpcserver"
vtadminhttp "vitess.io/vitess/go/vt/vtadmin/http"
Expand All @@ -47,6 +48,8 @@ var (
enableRBAC bool
disableRBAC bool

cacheRefreshKey string

traceCloser io.Closer = &noopCloser{}

rootCmd = &cobra.Command{
Expand Down Expand Up @@ -129,6 +132,11 @@ func run(cmd *cobra.Command, args []string) {
clusters[i] = cluster
}

if cacheRefreshKey == "" {
log.Warningf("no cache-refresh-key set; forcing cache refreshes will not be possible")
}
cache.SetCacheRefreshKey(cacheRefreshKey)

s := vtadmin.NewAPI(clusters, vtadmin.Options{
GRPCOpts: opts,
HTTPOpts: httpOpts,
Expand Down Expand Up @@ -185,6 +193,12 @@ func main() {
rootCmd.Flags().BoolVar(&enableRBAC, "rbac", false, "whether to enable RBAC. must be set if not passing --rbac")
rootCmd.Flags().BoolVar(&disableRBAC, "no-rbac", false, "whether to disable RBAC. must be set if not passing --no-rbac")

// Global cache flags (N.B. there are also cluster-specific cache flags)
cacheRefreshHelp := "instructs a request to ignore any cached data (if applicable) and refresh the cache;" +
"usable as an HTTP header named 'X-<key>' and as a gRPC metadata key '<key>'\n" +
"Note: any whitespace characters are replaced with hyphens."
rootCmd.Flags().StringVar(&cacheRefreshKey, "cache-refresh-key", "vt-cache-refresh", cacheRefreshHelp)

// glog flags, no better way to do this
rootCmd.Flags().AddGoFlag(flag.Lookup("v"))
rootCmd.Flags().AddGoFlag(flag.Lookup("logtostderr"))
Expand Down
22 changes: 22 additions & 0 deletions go/vt/vtadmin/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,28 @@ func NewAPI(clusters []*cluster.Cluster, opts Options) *API {
return api
}

// Close closes all the clusters in an API concurrently. Its primary function is
// to gracefully shutdown cache background goroutines to avoid data races in
// tests, but needs to be exported to be called by those tests. It does not have
// any production use case.
func (api *API) Close() error {
var (
wg sync.WaitGroup
rec concurrency.AllErrorRecorder
)

for _, c := range api.clusters {
wg.Add(1)
go func(c *cluster.Cluster) {
defer wg.Done()
rec.RecordError(c.Close())
}(c)
}

wg.Wait()
return rec.Error()
}

// ListenAndServe starts serving this API on the configured Addr (see
// grpcserver.Options) until shutdown or irrecoverable error occurs.
func (api *API) ListenAndServe() error {
Expand Down
49 changes: 47 additions & 2 deletions go/vt/vtadmin/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"

"vitess.io/vitess/go/vt/grpccommon"
"vitess.io/vitess/go/vt/topo"
Expand Down Expand Up @@ -99,6 +100,22 @@ func TestFindSchema(t *testing.T) {
},
},
},
FindAllShardsInKeyspaceResults: map[string]struct {
Response *vtctldatapb.FindAllShardsInKeyspaceResponse
Error error
}{
"testkeyspace": {
Response: &vtctldatapb.FindAllShardsInKeyspaceResponse{
Shards: map[string]*vtctldatapb.Shard{
"-": {
Shard: &topodatapb.Shard{
IsPrimaryServing: true,
},
},
},
},
},
},
},
Tablets: []*vtadminpb.Tablet{
{
Expand All @@ -108,6 +125,7 @@ func TestFindSchema(t *testing.T) {
Uid: 100,
},
Keyspace: "testkeyspace",
Shard: "-",
},
State: vtadminpb.Tablet_SERVING,
},
Expand Down Expand Up @@ -530,6 +548,7 @@ func TestFindSchema(t *testing.T) {
}

api := NewAPI(clusters, Options{})
defer api.Close()

resp, err := api.FindSchema(ctx, tt.req)
if tt.shouldErr {
Expand Down Expand Up @@ -739,6 +758,8 @@ func TestFindSchema(t *testing.T) {
)

api := NewAPI([]*cluster.Cluster{c1, c2}, Options{})
defer api.Close()

schema, err := api.FindSchema(ctx, &vtadminpb.FindSchemaRequest{
Table: "testtable",
TableSizeOptions: &vtadminpb.GetSchemaTableSizeOptions{
Expand Down Expand Up @@ -773,6 +794,9 @@ func TestFindSchema(t *testing.T) {
}

if schema != nil {
// Clone so our mutation below doesn't trip the race detector.
schema = proto.Clone(schema).(*vtadminpb.Schema)

for _, td := range schema.TableDefinitions {
// Zero these out because they're non-deterministic and also not
// relevant to the final result.
Expand Down Expand Up @@ -1378,7 +1402,6 @@ func TestGetSchema(t *testing.T) {
Name: "testtable",
},
},
TableSizes: map[string]*vtadminpb.Schema_TableSize{},
},
shouldErr: false,
},
Expand Down Expand Up @@ -1528,6 +1551,7 @@ func TestGetSchema(t *testing.T) {
Tablets: tt.tablets,
})
api := NewAPI([]*cluster.Cluster{c}, Options{})
defer api.Close()

resp, err := api.GetSchema(ctx, tt.req)
if tt.shouldErr {
Expand All @@ -1536,6 +1560,11 @@ func TestGetSchema(t *testing.T) {
return
}

if resp != nil {
// Clone so our mutation below doesn't trip the race detector.
resp = proto.Clone(resp).(*vtadminpb.Schema)
}

assert.NoError(t, err)
assert.Equal(t, tt.expected, resp)
})
Expand Down Expand Up @@ -1654,6 +1683,8 @@ func TestGetSchema(t *testing.T) {
)

api := NewAPI([]*cluster.Cluster{c1, c2}, Options{})
defer api.Close()

schema, err := api.GetSchema(ctx, &vtadminpb.GetSchemaRequest{
ClusterId: c1.ID,
Keyspace: "testkeyspace",
Expand Down Expand Up @@ -1690,6 +1721,9 @@ func TestGetSchema(t *testing.T) {
}

if schema != nil {
// Clone so our mutation below doesn't trip the race detector.
schema = proto.Clone(schema).(*vtadminpb.Schema)

for _, td := range schema.TableDefinitions {
// Zero these out because they're non-deterministic and also not
// relevant to the final result.
Expand Down Expand Up @@ -2204,6 +2238,7 @@ func TestGetSchemas(t *testing.T) {
}

api := NewAPI(clusters, Options{})
defer api.Close()

resp, err := api.GetSchemas(ctx, tt.req)
require.NoError(t, err)
Expand Down Expand Up @@ -2424,6 +2459,8 @@ func TestGetSchemas(t *testing.T) {
)

api := NewAPI([]*cluster.Cluster{c1, c2}, Options{})
defer api.Close()

resp, err := api.GetSchemas(ctx, &vtadminpb.GetSchemasRequest{
TableSizeOptions: &vtadminpb.GetSchemaTableSizeOptions{
AggregateSizes: true,
Expand Down Expand Up @@ -2491,14 +2528,22 @@ func TestGetSchemas(t *testing.T) {
}

if resp != nil {
for _, schema := range resp.Schemas {
// Clone schemas so our mutations below don't trip the race detector.
schemas := make([]*vtadminpb.Schema, len(resp.Schemas))
for i, schema := range resp.Schemas {
schema := proto.Clone(schema).(*vtadminpb.Schema)

for _, td := range schema.TableDefinitions {
// Zero these out because they're non-deterministic and also not
// relevant to the final result.
td.RowCount = 0
td.DataLength = 0
}

schemas[i] = schema
}

resp.Schemas = schemas
}

assert.NoError(t, err)
Expand Down
Loading