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
*: Add the mock owner-manager and schema-syncer #3359
Conversation
ddl/etcd_worker.go
Outdated
// CampaignOwners campaigns the DDL owner and the background owner. | ||
CampaignOwners(ctx goctx.Context, wg *sync.WaitGroup) error | ||
|
||
SchemaSyncer |
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.
Move this to the beginning of the interface.
ddl/etcd_worker.go
Outdated
// ChangeOwnerInNewWay is used for testing. | ||
var ChangeOwnerInNewWay = false | ||
// EtcdWorker is an interface for DDL access the etcd. | ||
type EtcdWorker interface { |
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.
How about rename it to DDLSyncer
.
DDL workers use DDLSyncer to synchronize ddl information. Using etcd to synchronize ddl information is an implementation of DDLSyncer. So put etcd
in the name of the interface is not proper.
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.
It also used to elect ddl leader, so Syncer
is not accuracy.
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.
But it's also used to campaign the owners. So DDLSyncer
may not be good.
ddl/mock_etcd_worker.go
Outdated
|
||
// mockEtcdWorker represents the structure which is used for electing owner. | ||
// It's used for local store and testing. | ||
// So this worker always the ddl owner and background owner. |
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.
How to test the scenario with multiple ddl workers?
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.
Integration testing, I guess.
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.
We have the DDL integration tests to test the multiple DDL workers.
The original test is only one is measured this situation. And Do we need to mock a object to mock this test?
@tiancaiamao PTAL |
ddl/ddl.go
Outdated
@@ -163,8 +163,8 @@ type ddl struct { | |||
hook Callback | |||
hookMu sync.RWMutex | |||
store kv.Storage | |||
// worker is used for electing the owner. | |||
worker *worker | |||
// worker is used for DDL access the etcd. |
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.
s/access/to access
ddl/ddl.go
Outdated
@@ -320,7 +319,7 @@ func (d *ddl) close() { | |||
} | |||
|
|||
close(d.quitCh) | |||
d.worker.cancel() | |||
d.worker.Cancel() |
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 prefer NewEtcdWorker
with d.quitCh
, so when close(d.quitCh)
, the etcd worker will get notification too.
defer d2.Stop() | ||
|
||
// Change the DDL owner. | ||
d1.Stop() |
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.
why remove those code?
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.
Because the mock etcd worker is the local store, this worker must be the owner.
ddl/etcd_worker.go
Outdated
// ChangeOwnerInNewWay is used for testing. | ||
var ChangeOwnerInNewWay = false | ||
// EtcdWorker is an interface for DDL access the etcd. | ||
type EtcdWorker interface { |
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.
It also used to elect ddl leader, so Syncer
is not accuracy.
ddl/etcd_worker.go
Outdated
SchemaSyncer | ||
|
||
// Cancel cancels this etcd worker campaign. | ||
Cancel() |
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.
we don't need this, really...
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.
cancel and close is not the same thing
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.
Yep, so I cancel the campaign operation.
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.
So, can this function be called multiple times?
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.
This question we discussed.
ddl/etcd_worker.go
Outdated
func (d *ddl) campaignOwners(ctx goctx.Context) error { | ||
err := d.worker.newSession(ctx, newSessionDefaultRetryCnt) | ||
// CampaignOwners implements EtcdWorker.CampaignOwners interface. | ||
func (w *worker) CampaignOwners(ctx goctx.Context, wg *sync.WaitGroup) error { |
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.
why wg
not a member of worker, but pass as argument instead?
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.
It uses the DDL's wg
.
ddl/mock_etcd_worker.go
Outdated
|
||
// mockEtcdWorker represents the structure which is used for electing owner. | ||
// It's used for local store and testing. | ||
// So this worker always the ddl owner and background owner. |
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.
Integration testing, I guess.
ddl/mock_etcd_worker.go
Outdated
} | ||
|
||
// Init implements SchemaSyncer.Init interface. | ||
func (s *mockSchemaSyncer) Init(ctx goctx.Context) error { |
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.
You can mock a SchemaSyncer
, without etcd client.
m, err := d.Stats() | ||
c.Assert(err, IsNil) | ||
c.Assert(m[ddlOwnerID], Equals, d.uuid) | ||
// TODO: Get this information from etcd. |
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.
Is it changed in this PR?
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.
Not yet. It will do at the next PR.
PTAL @tiancaiamao @coocood @shenli |
ddl/etcd_worker.go
Outdated
for { | ||
select { | ||
case <-worker.etcdSession.Done(): | ||
case <-w.etcdSession.Done(): | ||
// TODO: Create session again? |
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.
Should we fix this TODO before rc3
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.
ddl/etcd_worker.go
Outdated
worker.watchOwner(ctx, string(resp.Kvs[0].Key)) | ||
worker.setOwnerVal(key, false) | ||
d.hookMu.Lock() | ||
d.hook.OnWatched(ctx) |
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.
Why remove this hook?
ddl/schema_test.go
Outdated
@@ -216,21 +216,19 @@ func (s *testSchemaSuite) TestSchemaWaitJob(c *C) { | |||
ctx := testNewContext(d2) | |||
|
|||
// d2 must not be owner. | |||
testCheckOwner(c, d2, false, ddlJobFlag) | |||
d2.worker.SetOwner(false) |
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.
Should we check if d2.worker.IsOwner()?
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.
It was checked in line227.
|
||
schemaID, err := d2.genGlobalID() | ||
c.Assert(err, IsNil) | ||
doDDLJobErr(c, schemaID, 0, model.ActionCreateSchema, []interface{}{dbInfo}, ctx, d2) | ||
|
||
// d2 must not be owner. | ||
testCheckOwner(c, d2, false, ddlJobFlag) |
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.
Should we check this?
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.
d2
uses a mock worker,it only changes the owner state by us,so I don't think it's necessary to check.
@tiancaiamao @coocood PTAL |
ddl/etcd_worker.go
Outdated
// ChangeOwnerInNewWay is used for testing. | ||
var ChangeOwnerInNewWay = false | ||
// EtcdWorker is an interface for DDL to access the etcd. | ||
type EtcdWorker interface { |
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.
Since it is an interface, it doesn't have to be etcd, etcd it's the implementation.
I think separate it into two interfaces is better.
One is SchemaSyncer
, the other one is OwnerManager
.
ddl/ddl.go
Outdated
// If etcdCli is nil, it's the local store, so use the mockOwnerManager and mockSchemaSyncer. | ||
// It's always used for testing. | ||
if etcdCli == nil { | ||
manager = NewMockOwnerManager(etcdCli, id, cancelFunc) |
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.
Why pass nil etcdCli to create mock manager and mock sycner?
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.
Because if it's the local store, domain's etcdCli is nil. It's original logic.
ddl/mock.go
Outdated
} | ||
|
||
// NewMockOwnerManager creates a new mock OwnerManager. | ||
func NewMockOwnerManager(etcdCli *clientv3.Client, id string, cancel goctx.CancelFunc) OwnerManager { |
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.
The first argument is not used.
ddl/mock.go
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// Package ddl is just for test only. |
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.
This comment confuses me.
ddl/mock.go
Outdated
|
||
// mockOwnerManager represents the structure which is used for electing owner. | ||
// It's used for local store and testing. | ||
// So this worker always the ddl owner and background owner. |
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.
will always be
ddl/mock.go
Outdated
} | ||
|
||
// CampaignOwners implements mockOwnerManager.CampaignOwners interface. | ||
func (m *mockOwnerManager) CampaignOwners(_ goctx.Context, _ *sync.WaitGroup) error { return 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.
Need to set m.ddlOwner to 1?
ddl/owner_manager.go
Outdated
|
||
// ChangeOwnerInNewWay is used for controlling the way of changing owner. | ||
// TODO: Remove it. | ||
var ChangeOwnerInNewWay = true |
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.
Seems this is never used.
PTAL @shenli @coocood @tiancaiamao |
LGTM |
ddl/mock.go
Outdated
|
||
// OwnerCheckAllVersions implements SchemaSyncer.OwnerCheckAllVersions interface. | ||
func (s *mockSchemaSyncer) OwnerCheckAllVersions(ctx goctx.Context, latestVer int64) error { | ||
for { |
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.
ticker := time.NewTicker(mockCheckVersInterval)
defer ticker.Close()
select {
case <-ctx.Done():
case <-ticker.C:
ver := atomic.LoadInt64(&s.selfSchemaVersion)
if ver == latestVer {
return nil
}
}
ddl/syncer.go
Outdated
return s.globalVerCh | ||
} | ||
|
||
// UpdateSelfVersion implements SchemaSyncer.UpdateSelfVersion interface. | ||
func (s *schemaVersionSyncer) UpdateSelfVersion(ctx goctx.Context, version int64) error { | ||
ver := strconv.FormatInt(version, 10) | ||
return s.putKV(ctx, putKeyNoRetry, s.selfSchemaVerPath, ver) |
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.
It should put with a lease option, so if RemoveSelfVersionPath
fail, the path will be remove automatically.
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.
This will do for the next PR.
PTAL @shenli @tiancaiamao |
LGTM |
This PR add the interface of owner-manager and schema-syncer. Besides mocks owner-manager and schema-syncer. We use these interfaces instead of the old way.
The next PR I will remove the old code and add some tests.