Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
29 changes: 20 additions & 9 deletions go/vt/discovery/tablet_stats_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ type TabletStatsCache struct {
// Note we keep track of all master tablets in all cells.
cell string

// cell to region mapping function
cellToRegion func(cell string) string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this can just be region string. The region for a cell should never change. So, it can be precomputed and initialized at construction.


// mu protects the entries map. It does not protect individual
// entries in the map.
mu sync.RWMutex
Expand All @@ -65,8 +68,8 @@ type tabletStatsCacheEntry struct {
// Note we do the registration in this code to guarantee we call
// SetListener with sendDownEvents=true, as we need these events
// to maintain the integrity of our cache.
func NewTabletStatsCache(hc HealthCheck, cell string) *TabletStatsCache {
return newTabletStatsCache(hc, cell, true /* setListener */)
func NewTabletStatsCache(hc HealthCheck, cell string, cellToRegion func(cell string) string) *TabletStatsCache {
return newTabletStatsCache(hc, cell, cellToRegion, true /* setListener */)
}

// NewTabletStatsCacheDoNotSetListener is identical to NewTabletStatsCache
Expand All @@ -76,14 +79,22 @@ func NewTabletStatsCache(hc HealthCheck, cell string) *TabletStatsCache {
// When the caller sets its own listener on "hc", they must make sure that they
// set the parameter "sendDownEvents" to "true" or this cache won't properly
// remove tablets whose tablet type changes.
func NewTabletStatsCacheDoNotSetListener(cell string) *TabletStatsCache {
return newTabletStatsCache(nil, cell, false /* setListener */)
func NewTabletStatsCacheDoNotSetListener(cell string, cellToRegion func(cell string) string) *TabletStatsCache {
return newTabletStatsCache(nil, cell, cellToRegion, false /* setListener */)
}

// UpdateCellsToRegions is mainly for testing purpose, the `cellsToRegions` mapping should be provided in the constructor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it's a test-only function, it should be moved to a _test.go file. But I think you don't need this function. You should be able directly update the region if you want to change it for testing purposes.

func (tc *TabletStatsCache) UpdateCellsToRegions(cellsToRegions map[string]string) {
tc.cellToRegion = func(cell string) string {
return cellsToRegions[cell]
}
}

func newTabletStatsCache(hc HealthCheck, cell string, setListener bool) *TabletStatsCache {
func newTabletStatsCache(hc HealthCheck, cell string, cellToRegion func(cell string) string, setListener bool) *TabletStatsCache {
tc := &TabletStatsCache{
cell: cell,
entries: make(map[string]map[string]map[topodatapb.TabletType]*tabletStatsCacheEntry),
cell: cell,
cellToRegion: cellToRegion,
entries: make(map[string]map[string]map[topodatapb.TabletType]*tabletStatsCacheEntry),
}

if setListener {
Expand Down Expand Up @@ -144,8 +155,8 @@ func (tc *TabletStatsCache) getOrCreateEntry(target *querypb.Target) *tabletStat

// StatsUpdate is part of the HealthCheckStatsListener interface.
func (tc *TabletStatsCache) StatsUpdate(ts *TabletStats) {
if ts.Target.TabletType != topodatapb.TabletType_MASTER && ts.Tablet.Alias.Cell != tc.cell {
// this is for a non-master tablet in a different cell, drop it
if ts.Target.TabletType != topodatapb.TabletType_MASTER && ts.Tablet.Alias.Cell != tc.cell && tc.cellToRegion(ts.Tablet.Alias.Cell) != tc.cellToRegion(tc.cell) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here we have a couple of options:

  1. Extract the region from the cell info
  2. Denormalize the region into the tablet record

@alainjobart any preferences?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You also need to exclude empty regions.

// this is for a non-master tablet in a different cell and a different region, drop it
return
}

Expand Down
2 changes: 1 addition & 1 deletion go/vt/discovery/tablet_stats_cache_wait_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func TestWaitForTablets(t *testing.T) {
createFakeConn(tablet, input)

hc := NewHealthCheck(1*time.Millisecond, 1*time.Hour)
tsc := NewTabletStatsCache(hc, "cell")
tsc := NewTabletStatsCache(hc, "cell", func(cell string) string { return cell })
hc.AddTablet(tablet, "")

// this should time out
Expand Down
152 changes: 81 additions & 71 deletions go/vt/proto/topodata/topodata.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions go/vt/topo/cell_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ func (ts *Server) GetCellInfo(ctx context.Context, cell string, strongRead bool)
return ci, nil
}

// GetRegionByCell gets the region from a given cell
func (ts *Server) GetRegionByCell(ctx context.Context, cell string) (string, error) {

cellInfo, err := ts.GetCellInfo(ctx, cell, false)
if err == nil {
return cellInfo.Region, nil
}
return "", err
}

// CreateCellInfo creates a new CellInfo with the provided content.
func (ts *Server) CreateCellInfo(ctx context.Context, cell string, ci *topodatapb.CellInfo) error {
// Pack the content.
Expand Down
22 changes: 22 additions & 0 deletions go/vt/topo/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,28 @@ func (ts *Server) ConnForCell(ctx context.Context, cell string) (Conn, error) {
return conn, nil
}

// CellToRegionMapper function is a wrapper around topo.Server#GetRegionByCell with caching and error handling

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see the benefit of using this pattern, vs just having a global map and a function to get the value from the map directly. Also, the global function backed by a global map would be easy to unit test. And then you wouldn't need to pass it everywhere. Overall, the number of lines of code for this change would go down dramatically.

Also, this needs to be thread-safe, so please protect the map with a mutex.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agree that a global map would be simpler, i actually attempted that in the 1st round, but had difficulty collecting all the cells upfront to init the map. i thought i might use GetKnownCells for that purpose, but not sure how to capture new cell add event (though rare enough in practice).
for the reason above, i chose the lambda to make it lazy, and able to handle new cell event uniformly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you need to collect anything upfront? You start with an empty map and a mutex:

var cellToRegionMap map[string]string
var cellToRegionMapMu sync.Mutex

and export a function that does the usual Mutex lock, get from the cache, if not there get from topo service, and populate the cache.

The population only happens the first time you call the function, there is nothing upfront needed... You can also use a RW Mutex and do the lookup holding just the Read lock.

It's not different from what you're doing, except it's a global cache?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

func (ts *Server) CellToRegionMapper() func(cell string) string {

memoize := make(map[string]string)
ctx := context.Background()

return func(cell string) string {
if ts == nil {
return cell
}
if region, ok := memoize[cell]; ok {
return region
}
if region, err := ts.GetRegionByCell(ctx, cell); err == nil && region != "" {
memoize[cell] = region
return region
}
// for backward compatibility, when region isn't available, it's the same as given cell
return cell
}
}

// Close will close all connections to underlying topo Server.
// It will nil all member variables, so any further access will panic.
func (ts *Server) Close() {
Expand Down
Loading