tabletserver: improved state transitions with new stateManager #6396
Conversation
go/vt/dbconfigs/dbconfigs.go
Outdated
| @@ -190,6 +191,10 @@ func (c Connector) Connect(ctx context.Context) (*mysql.Conn, error) { | |||
|
|
|||
| // MysqlParams returns the connections params | |||
| func (c Connector) MysqlParams() (*mysql.ConnParams, error) { | |||
| if c.connParams == nil { | |||
| // This is only possible during tests. | |||
| return nil, fmt.Errorf("parameters are empty") | |||
| "sync" | ||
| "time" | ||
|
|
||
| "golang.org/x/net/context" |
There was a problem hiding this comment.
"golang.org/x/net/context" -> "context"
| const ( | ||
| // StateNotConnected is the state where tabletserver is not | ||
| // connected to an underlying mysql instance. | ||
| StateNotConnected = iota |
There was a problem hiding this comment.
It would be really nice if states are not just plain int64, but their own type, built on int64. It helps understanding and gives compiler protection for a bunch of situations.
| assert.Equal(t, int64(StateNotConnected), sm.State()) | ||
| } | ||
|
|
||
| func verifySubcomponent(t *testing.T, component interface{}, order int64, state testState) { |
There was a problem hiding this comment.
super-nit: I think it would be nice if we changed the order of these, so that order comes before component. That way, assertions line up better:
verifySubcomponent(t, 1, sm.messager, testStateClosed)
verifySubcomponent(t, 2, sm.te, testStateClosed)
verifySubcomponent(t, 3, sm.txThrottler, testStateClosed)
verifySubcomponent(t, 4, sm.qe, testStateClosed)
verifySubcomponent(t, 5, sm.watcher, testStateClosed)
...
c42ab86 to
c6d295f
Compare
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Tests still need fixing Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Looks like tm explicitly sets it expecting tablet server to unset it. We'll need to revisit this. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Also address a few early comments. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
If the requested type is RESTORE, force StateNotConnected. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Some tests became flaky because they expect the state to change immediately after a call. This change brings the behavior to be closer to the existing one. But the tabletserver will continue to retry if an request failed. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
| // InitDBConfig initializes the target parameters for the Engine. | ||
| func (vse *Engine) InitDBConfig(keyspace string) { | ||
| vse.keyspace = keyspace | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: this can be renamed to setKeyspace as it is only setting keyspace.
| // InitDBConfig initializes the target parameters for the throttler. | ||
| func (t *TxThrottler) InitDBConfig(target querypb.Target) { | ||
| t.target = target | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: InitDBConfig -> setTarget
| // Open must be called before sending requests to QueryEngine. | ||
| func (qe *QueryEngine) Open() error { | ||
| if qe.isOpen { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Should this check be not performed under lock or is it protected by higher order?
There was a problem hiding this comment.
The state manager ensures that Open and Close are not called concurrently. A lock is needed only if the variable is accessed by other functions, which is the case in the other situations.
There are a few places where I do this check inside a lock even though it's not needed. No harm.
| case <-tmr.C: | ||
| log.Fatal("Shutdown took too long. Crashing") |
There was a problem hiding this comment.
This can't be tested because it will crash the test :).
There was a problem hiding this comment.
The InitDBConfig is a naming convention that implies the completion of an incomplete "New" instantiation. So, giving it other meaningful names will cause this semantic to get lost.
I'm hoping to get rid of this two-phase initialization as part of my refactor. Let's see if I succeed.
| // Open must be called before sending requests to QueryEngine. | ||
| func (qe *QueryEngine) Open() error { | ||
| if qe.isOpen { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The state manager ensures that Open and Close are not called concurrently. A lock is needed only if the variable is accessed by other functions, which is the case in the other situations.
There are a few places where I do this check inside a lock even though it's not needed. No harm.
| case <-tmr.C: | ||
| log.Fatal("Shutdown took too long. Crashing") |
There was a problem hiding this comment.
This can't be tested because it will crash the test :).
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
The tabletserver can now remember its desired state and continuously retry if the first attempt fails. This responsibility was previously with tabletmanager. This change allows us to delete the corresponding code from tabletmanager.
Most sub-component function calls have been normalized to simplify state management. All such functions are now idempotent.
The new stateManager invokes the necessary functions to reach any required state knowing that the functions are idempotent. This allows it to act in a manner that is agnostic of what the current state is.
Strict ordering rules have been defined for how to open and close various services.