region/cell feature draft by inexplicable · Pull Request #3460 · vitessio/vitess

inexplicable · 2017-12-08T19:06:38Z

This is a PR addressing: #3355

The major changes include:

added region field to cell_info, updated vtctld commands to support region
added GetRegionByCell and some helpers to topo.Server to query region of any given cell

NOTE that, if no region info is available, it uses cell to return for backward compatibility

modified TabletStatsCache#StatsUpdate to accept cross cell slave tablets
modified discoverygateway#shuffleTablets to favor same cell tablets

googlebot · 2017-12-08T19:06:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

inexplicable · 2017-12-08T21:43:12Z

I signed it!

sougou

We should wait for @alainjobart to also take a look at this. I've have some initial feedback. I'm still not sure about the shuffle function. I'm wondering if we need to keep 'other cells' in a separate data structure to avoid repeated sorting.

sougou · 2017-12-10T22:49:02Z

go/vt/discovery/tablet_stats_cache.go

 	cell string

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


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.

sougou · 2017-12-10T22:51:02Z

go/vt/discovery/tablet_stats_cache.go

+	return newTabletStatsCache(nil, cell, cellToRegion, false /* setListener */)
+}
+
+// UpdateCellsToRegions is mainly for testing purpose, the `cellsToRegions` mapping should be provided in the constructor


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.

sougou · 2017-12-10T22:58:25Z

go/vt/discovery/tablet_stats_cache.go

 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) {


Here we have a couple of options:

Extract the region from the cell info

Denormalize the region into the tablet record

@alainjobart any preferences?

sougou · 2017-12-10T23:00:32Z

go/vt/vtgate/gateway/discoverygateway.go

-		index = rand.Intn(i + 1)
-		tablets[i], tablets[index] = tablets[index], tablets[i]
+
+	//move all same cell tablets to the front, this is O(n)


Go thing: need space between // and start of comment. Running gofmt should fix this. Some pedantic people may also insist on punctuation.

sougou · 2017-12-10T23:03:53Z

go/vt/vtgate/gateway/discoverygateway.go


-func shuffleTablets(tablets []discovery.TabletStats) {
-	index := 0
+func shuffleTablets(cell string, tablets []discovery.TabletStats) {


Need unit tests for this function, to test all code paths.

👍 ut added to discoverygateway_test

sougou · 2017-12-10T23:05:17Z

go/vt/discovery/tablet_stats_cache.go

 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) {


You also need to exclude empty regions.

alainjobart

I like the feature.

The implementation can be a lot simpler though, just put a global map and a global function in go/vt/topo, and use that where you need to.

alainjobart · 2017-12-11T17:17:38Z

proto/topodata.proto

  // to server_address.
  string root = 2;
+
+  // Region is a group this cell belongs to


Please be a bit more specific in this comment (and end it with a period). Something like:

// Region is a group this cell belongs to. Used by vtgate to route traffic to
// other cells when there is no available tablet in the current region.

alainjobart · 2017-12-11T17:20:25Z

go/vt/topo/server.go

 	return conn, nil
 }

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


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.

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.

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?

sougou · 2017-12-15T19:25:22Z

@alainjobart is this good to go?

alainjobart · 2017-12-15T19:40:03Z

Looking at this, I don't like how all the objects fit together, with topo.Open() remembering the topo.Server, and then the other methods using that later. I think the following would work better:

create a RegionCache object, with the mutex, the map, and a *topo.Server. Use the *topo.Server to query the CellInfo when needed.
when creating the TableStatsCache, give it the *topo.Server, and have it create a RegionCache. Use it as appropriate.
I think that would organize the code better, and limit global variables.

Also, the context.Background calls should be avoided, if possible, by passing in the context when necessary. I am not sure how to plumb this one though.

This is a bunch of changes, and in other parts of the code too, let me know if you think that's too much for this PR, and we can do it in another.

alainjobart

Looks better, thanks for making these changes!

alainjobart · 2017-12-17T01:52:07Z

go/vt/discovery/tablet_stats_cache.go

 // 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, ts *topo.Server) *TabletStatsCache {


One minor comment here: the parameter order should probably be hc, ts, cell (as topo.Server is more general, and cell a specialization). Would match the order of the other method below too.

👍 updating PR

alainjobart · 2017-12-17T01:52:18Z

go/vt/discovery/tablet_stats_cache.go

 }

-func newTabletStatsCache(hc HealthCheck, cell string, setListener bool) *TabletStatsCache {
+func newTabletStatsCache(hc HealthCheck, cell string, ts *topo.Server, setListener bool) *TabletStatsCache {


same thing here, ts then cell.

alainjobart · 2017-12-17T01:53:12Z

go/vt/topo/server.go

+}
+
+// UpdateCellsToRegions overwrites the global map built by topo server init, and is meant for testing purpose only.
+func UpdateCellsToRegions(cellsToRegions map[string]string) {


If it's only for tests, should be named UpdateCellsToRegionsForTests

…egions` as `UpdateCellsToRegionsForTests`

sougou · 2017-12-17T19:03:32Z

go/vt/discovery/tablet_stats_cache.go

 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.getRegionByCell(ts.Tablet.Alias.Cell) != tc.getRegionByCell(tc.cell) {


Shouldn't you check for blank region here? Otherwise, if region is not specified, it will end up being false always, and end up including all cells in the stats.

actually not, as the getRegionByCell won't ever return empty region, it returns the cell value in this case. as agreed for backward compatibility

of course :)

inexplicable · 2017-12-18T18:52:35Z

I signed it!

googlebot · 2017-12-18T18:52:38Z

CLAs look good, thanks!

inexplicable added 2 commits December 8, 2017 10:53

region/cell feature

482cf3e

do not remember empty region

9750d35

googlebot added the cla: no label Dec 8, 2017

inexplicable mentioned this pull request Dec 8, 2017

proposal: cell/region topology configuration to improve replica query performance #3355

Closed

sougou requested a review from alainjobart December 10, 2017 22:51

sougou reviewed Dec 10, 2017

View reviewed changes

alainjobart reviewed Dec 11, 2017

View reviewed changes

inexplicable added 3 commits December 13, 2017 12:48

refactored based on feedback, using global map/func instead

4cde6d4

restore lazy style cell to region lookup

a0db555

doc updates

e0ae56b

rafael mentioned this pull request Dec 14, 2017

region feature rewrite after getting upstream feedback in tinyspeck/vitess#57

Merged

let ts *topo.Server be a param to GetRegionByCell

e97d6d4

alainjobart reviewed Dec 17, 2017

View reviewed changes

refactored to put ts ahead of cell param, renamed `UpdateCellsToR…

a2e9d0d

…egions` as `UpdateCellsToRegionsForTests`

alainjobart approved these changes Dec 17, 2017

View reviewed changes

sougou reviewed Dec 17, 2017

View reviewed changes

googlebot added cla: yes and removed cla: no labels Dec 18, 2017

sougou merged commit 82ab856 into vitessio:master Dec 18, 2017

rafael deleted the upstream-region branch October 17, 2018 17:18

This was referenced Dec 16, 2018

[vt/discovery] Add region context in GetAggregateStats #4459

Closed

[topology] Deprecate Shard cells #4482

Closed

Conversation

inexplicable commented Dec 8, 2017

Uh oh!

googlebot commented Dec 8, 2017

Uh oh!

inexplicable commented Dec 8, 2017

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainjobart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sougou commented Dec 15, 2017

Uh oh!

alainjobart commented Dec 15, 2017

Uh oh!

alainjobart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inexplicable commented Dec 18, 2017

Uh oh!

googlebot commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants