*: Do retry when schema is expired #2094
Conversation
| @@ -47,19 +47,19 @@ type Domain struct { | |||
|
|
|||
| // loadInfoSchema loads infoschema at startTS into handle, usedSchemaVersion is the currently used | |||
| // infoschema version, if it is the same as the schema version at startTS, we don't need to reload again. | |||
| func (do *Domain) loadInfoSchema(handle *infoschema.Handle, usedSchemaVersion int64, startTS uint64) error { | |||
| func (do *Domain) loadInfoSchema(handle *infoschema.Handle, usedSchemaVersion int64, startTS uint64) (int64, error) { | |||
There was a problem hiding this comment.
Add comment for the first return value.
| @@ -282,6 +282,7 @@ func (do *Domain) Reload() error { | |||
| defer do.m.Unlock() | |||
|
|
|||
| var err error | |||
| var latestSchemaVersion int64 | |||
There was a problem hiding this comment.
var (
err error
latestSchemaVersion int64
)
There was a problem hiding this comment.
It doesn't reach three or more variables. I think there is no need to change this format.
…wait-valid Conflicts: domain/domain.go domain/domain_test.go
| @@ -471,14 +482,18 @@ func (s *schemaValidityInfo) SetValidity(v bool, lastSuccTS uint64) { | |||
| s.mux.Unlock() | |||
| } | |||
|
|
|||
| func (s *schemaValidityInfo) Check(txnTS uint64) error { | |||
| // Check checks schema validity. It returns the current schema version and an error. | |||
There was a problem hiding this comment.
Add comment about when this will be used, and when txnTS is 0.
| @@ -133,15 +134,32 @@ func (s *session) cleanRetryInfo() { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Set them as system variables. | |||
| var ( | |||
| checkSchemaValidityRetryTimes = 10 | |||
There was a problem hiding this comment.
10 seconds is a little too short.
coocood
changed the title
| @@ -433,52 +436,72 @@ func (m *MockFailure) getValue() bool { | |||
| } | |||
|
|
|||
| type schemaValidityInfo struct { | |||
There was a problem hiding this comment.
this struct is more and more complex and out of control on its way, I'm afraid
| MockReloadFailed MockFailure // It mocks reload failed. | ||
| } | ||
|
|
||
| func (s *schemaValidityInfo) updateSchemaVersion(version int64) { | ||
| atomic.StoreInt64(&s.lastSchemaVer, version) |
There was a problem hiding this comment.
I don't suggest use both atomic and RWMutex...
schemaValidityInfo is already a mess, it makes things from bad to worse.
If any bug caused by this style, it will be the most difficult bug to debug.
There was a problem hiding this comment.
Yep, Do you have some suggestions?
| mustExecSQL(c, se, "create table t1 (a int);") | ||
| mustExecSQL(c, se, "drop table if exists t2;") | ||
| mustExecSQL(c, se, "create table t2 (a int);") | ||
| startCh1 := make(chan struct{}, 0) |
There was a problem hiding this comment.
unbuffered channel don't need size 0.... make(chan struct{})
| // execute successfully | ||
| _, err := exec(s, "begin;") | ||
| <-start | ||
| <-start |
There was a problem hiding this comment.
why <-start two times?
There was a problem hiding this comment.
Control the order of execution.
|
LGTM |
| // txnTS != 0, it means the transition isn't nil. | ||
| // txnTS <= s.recoveredTS, it means the transition begins before schema is recovered. | ||
| // schemaVer != currVer, it means the schema version is changed. | ||
| if txnTS != 0 && txnTS <= s.recoveredTS && schemaVer != currVer { |
There was a problem hiding this comment.
I think txnTS <= s.recoveredTS condition should be removed ?
schema version change during a transaction, this transaction is invalid.
| } | ||
|
|
||
| // SetValidity sets the schema validity value. | ||
| // SetExpireInfo sets the schema validity value. |
There was a problem hiding this comment.
SetExpireInfo sets the schema expire info.
| // If the schema is invalid, we need to rollback the current transaction. | ||
| func (s *session) checkSchemaValidOrRollback() error { | ||
| var ts uint64 | ||
| if s.txn != nil { | ||
| ts = s.txn.StartTS() | ||
| } else { | ||
| s.schemaVerInCurrTxn = 0 |
There was a problem hiding this comment.
If s.txn is nil, should we return nil here? For we have nothing to rollback.
|
LGTM |
zimulala
changed the title
zimulala
changed the title
When the transaction encounters the invalid schema while it checks the validity of schema , we need to try again.
Make code more readable.