Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

[WIP] Add optimistic sharding mode #491

Closed
wants to merge 8 commits into from

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Feb 25, 2020

WIP! Borrowing the CI for testing.

What problem does this PR solve?

Add a simple optimistic DDL lock mode. Enable with dmctl set-ddl-lock-mode optimistic.

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@kennytm kennytm changed the base branch from ha-dev to master February 26, 2020 09:50
@kennytm kennytm force-pushed the kennytm/optimistic-sharding branch 3 times, most recently from b2b7095 to 257e59a Compare February 26, 2020 11:30
@kennytm kennytm force-pushed the kennytm/optimistic-sharding branch from 257e59a to 89c68f7 Compare February 26, 2020 17:35
@kennytm kennytm force-pushed the kennytm/optimistic-sharding branch 5 times, most recently from 5bfbc31 to 8e69560 Compare March 8, 2020 19:20
@kennytm kennytm force-pushed the kennytm/optimistic-sharding branch from 8e69560 to 3529f39 Compare March 8, 2020 20:36
@kennytm
Copy link
Contributor Author

kennytm commented Mar 8, 2020

ok i'm out of idea how the optimistic lock should fit into the existing system.

current pessimistic-only system is roughly like:

// master 1
go {
    for {
        task := <-infoCh 
        lock := FindLock(task)
        synced := lock.TrySync(task)
        if synced {
            opCh <- NewOperation(lock)
        }
    }
}    

// master 2
go {
    for {
        op := <-opCh 
        lock := FindLock(op)
        for _, source := range lock.Sources {
            if source == lock.Owner {
                clientCh <- "execute " + lock.DDLs
            } else {
                clientCh <- "skip " + lock.DDLs 
            }
        }
        RemoveLock(op)
    }
}

// client 
go {
    ...
    infoCh <- DDLs 
    action := <-clientCh  // (reader responsible for deleting the etcd keys??)
    ...
}

supporting optimistic-lock should be simply changing:

// master 1
go {
    for {
        task := <-infoCh 
        lock := FindLock(task)
        newDDLs, err := lock.TrySync(task)       // <-- changed 
        if err != nil {                          
            opCh <- NewOperation(task, newDDLs)
        }
    }
}    

// master 2
go {
    for {
        op := <-opCh 
        lock := FindLock(op)
        for _, source := range lock.Sources {
            if source == lock.Owner {
                clientCh <- "execute " + op.DDLs // <-- changed
            } else {
                clientCh <- "skip"               // <-- changed 
            }
        }
        if lock.Synced() {                       // <-- changed 
            RemoveLock(op)
        }
    }
}

// client 
go {
    ...
    infoCh <- DDLs 
    action := <-clientCh
    ...
}

but i can't get the client to accept the modified DDLs 😕

@csuzhangxc
Copy link
Member

in #522 I change only the leader of the master to coordinate shard DDL in pessimistic mode.

In pessimistic mode, the Info should be deleted by the worker, the Operation should be deleted by the master, you can see pkg/pessimism/doc.go.

)

// LockKeeper used to keep and handle DDL lock conveniently.
// The lock information do not need to be persistent, and can be re-constructed from the shard DDL info.
type LockKeeper struct {
mu sync.RWMutex
locks map[string]*Lock // lockID -> Lock
mode LockMode
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set a different mode for each migration task?

If so, maybe we can add a config item in task.yaml?

cmpRes, err := oldJoined.Compare(newJoined)
l.synced = err == nil && cmpRes == 0
if err != nil || cmpRes == 0 {
ddls = nil
Copy link
Member

Choose a reason for hiding this comment

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

set ddls = nil for err == nil && cmpRes == 0 (l.synced = true)?

@csuzhangxc
Copy link
Member

implemented in #568.

@csuzhangxc csuzhangxc closed this Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants