*: Reduce loading schema times #1984

zimulala · 2016-11-09T11:17:07Z

Only if it's success to do DDL or in reloadLoop to load schema
Update code
Improve test coverage from 65% to 75%
fix Make sure lastInvalidTS is a normal txn TS. #1571

tiancaiamao · 2016-11-09T12:26:55Z

domain/domain.go

+		}
+		do.SchemaValidity.setLastFailedTS(ver.Ver)
+		log.Errorf("[ddl] load schema err %v, retry again", errors.ErrorStack(err))
+		if atomic.LoadInt32(exit) == 1 {


this is not Go-style, I suggest make exit a channel:

select { case <-timeout: case <-exit: }

tiancaiamao · 2016-11-09T12:37:52Z

domain/domain.go

 var defaultMinReloadTimeout = 20 * time.Second

 // Reload reloads InfoSchema.
-func (do *Domain) Reload() error {
+func (do *Domain) reload() error {
 	// for test
 	if do.SchemaValidity.MockReloadFailed {


mixing the test code logic would hurt readability, can you find better way to do it?
such as extract to an interface and wrap the mocked code upon the real implementation

tiancaiamao · 2016-11-09T12:47:49Z

domain/domain.go

 				log.Errorf("[ddl] reload schema in loop err %v", errors.ErrorStack(err))
+				time.Sleep(mustReloadSleepTime)


after the time.Sleep, it would miss the do.leaseCh signal for a while, would this cause problem?

tiancaiamao · 2016-11-09T12:53:06Z

domain/domain_test.go

+	c.Assert(dd.GetLease(), Equals, 100*time.Millisecond)
+	dom.SetLease(0 * time.Millisecond)
+	c.Assert(dd.GetLease(), Equals, 100*time.Millisecond)
+	time.Sleep(100 * time.Millisecond)


what's the purpose of this line?

coocood · 2016-11-17T03:21:24Z

domain/domain.go

 	}
+
+	return errors.Trace(err)
 }

 // MustReload reloads the infoschema.
 // If reload error, it will hold whole program to guarantee data safe.
 // It's public in order to do the test.
 func (do *Domain) MustReload() error {


MustReload does nothing more than reload. We can remove this function, use reload instead.

zimulala · 2016-11-17T06:36:12Z

PTAL @coocood @tiancaiamao @shenli

…zimuxia/domain-reload Conflicts: store/tikv/store_test.go

tiancaiamao · 2016-11-18T08:39:40Z

domain/domain.go

-				return
+			log.Infof("[ddl] check validity in a loop, sub:%v, lease:%v, succ:%v, waitTime:%v",
+				sub, lease, lastSuccTS, waitTime)
+			timer.Reset(waitTime)


This is not the ideal usage of timer.Reset

see https://godoc.org/time#Timer.Reset

To reuse an active timer, always call its Stop method first and—if it had expired—drain the value from its channel.

Here doesn't need to add that judgment.

tiancaiamao · 2016-11-18T08:48:41Z

domain/domain.go

-		do.SchemaValidity.SetValidity(false)
-		return errors.Trace(err)
+			lease = newLease
+			timer.Reset(0)


I think line 344~350 can just one line:
lease = newLease

I don‘t want to wait a old lease

tiancaiamao · 2016-11-18T08:50:30Z

domain/domain.go

-	ticker := time.NewTicker(lease)
-	defer ticker.Stop()
+	ticker := time.NewTicker(lease / 4)
+	defer func() { ticker.Stop() }()


defer ticker.Stop()

Because ticker is reassigned in line 375 . It can prevent panic.

tiancaiamao · 2016-11-18T08:55:37Z

domain/domain.go

 }

 func (do *Domain) loadSchemaInLoop(lease time.Duration) {
-	ticker := time.NewTicker(lease)


the code is misleading, I suggest set lease / 4 outside...
let caller do it instead of callee

tiancaiamao · 2016-11-18T09:18:50Z

domain/domain.go


 // Reload reloads InfoSchema.
+// It's public in order to do the test.
 func (do *Domain) Reload() error {


I think sleep and retry in Reload is not a good idea.
There would be new ticker signal happen during this, and may cause Reload be called several times.

A better way is reload returns error and let loadSchemaInLoop handle the retry.

Reload will return an error.

coocood · 2016-11-18T10:36:37Z

domain/domain.go

 }

 // SetValidity sets the schema validity value.
 // It's public in order to do the test.
-func (s *schemaValidityInfo) SetValidity(v bool) {
+func (s *schemaValidityInfo) SetValidity(v bool, lastSuccTS uint64) {


Can we remove the lastSuccTS parameter, and set s.firstValidTS = s.lastSuccTS directly?

Setting s.firstValidTS = s.lastSuccTS may be concurrent.

zimulala · 2016-11-21T07:45:46Z

domain/domain.go

-	ticker := time.NewTicker(lease)
-	defer ticker.Stop()
+	ticker := time.NewTicker(minInterval(lease))
+	defer func() { ticker.Stop() }()


I will deal with it in the next PR.

zimulala · 2016-11-21T07:48:14Z

PTAL @tiancaiamao @coocood

coocood · 2016-11-21T12:18:07Z

LGTM

tiancaiamao · 2016-11-21T12:46:16Z

store/tikv/store_test.go

 	}()

 	wg.Wait()
+
+	// retry after 2 seconds
+	time.Sleep(2000 * time.Millisecond)


it seems server is busy just 60ms, is it necessary sleep so long here?

If server is busy， transaction will retry after 2s.

tiancaiamao · 2016-11-22T02:36:23Z

LGTM

zimulala · 2016-11-22T02:49:50Z

PTAL @disksing

zimulala added the WIP label Nov 9, 2016

tiancaiamao reviewed Nov 9, 2016

View reviewed changes

zimulala force-pushed the zimuxia/domain-reload branch 4 times, most recently from 0fb9dc0 to 5bd5a13 Compare November 15, 2016 10:17

zimulala added 2 commits November 16, 2016 09:39

*: only if it's success to do DDL or in reloadLoop to load schema

8571471

domain: improve test coverage

23ef4c1

zimulala force-pushed the zimuxia/domain-reload branch from 5bd5a13 to e29e0e7 Compare November 16, 2016 01:40

*: test

897715d

zimulala force-pushed the zimuxia/domain-reload branch from e29e0e7 to 897715d Compare November 17, 2016 03:01

zimulala removed the WIP label Nov 17, 2016

coocood reviewed Nov 17, 2016

View reviewed changes

*: address comments

8af9fba

zimulala force-pushed the zimuxia/domain-reload branch from 6e503b8 to 8af9fba Compare November 17, 2016 06:33

zimulala added 6 commits November 17, 2016 14:50

*: only if it's success to do DDL or in reloadLoop to load schema

9f166f3

domain: improve test coverage

25322a0

*: test

48db0c8

*: address comments

2e9ce62

store: merge master

4d552e1

Merge branch 'zimuxia/domain-reload' of github.com:pingcap/tidb into …

f8b5e5f

…zimuxia/domain-reload Conflicts: store/tikv/store_test.go

tiancaiamao reviewed Nov 18, 2016

View reviewed changes

coocood reviewed Nov 18, 2016

View reviewed changes

domain: address comments

6aa69b5

zimulala commented Nov 21, 2016

View reviewed changes

tiancaiamao reviewed Nov 21, 2016

View reviewed changes

zimulala changed the title *: Reduce loading schema times [DNM]*: Reduce loading schema times Nov 22, 2016

*: remove TestBusyServerCop

ab32b51

zimulala force-pushed the zimuxia/domain-reload branch from 1432c16 to ab32b51 Compare November 22, 2016 09:49

zimulala changed the title ~~[DNM]*: Reduce loading schema times~~ *: Reduce loading schema times Nov 22, 2016

Merge branch 'master' into zimuxia/domain-reload

a23e718

zimulala merged commit 1181acc into master Nov 22, 2016

zimulala deleted the zimuxia/domain-reload branch November 22, 2016 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Reduce loading schema times #1984

*: Reduce loading schema times #1984

zimulala commented Nov 9, 2016 •

edited

Loading

tiancaiamao Nov 9, 2016

tiancaiamao Nov 9, 2016

tiancaiamao Nov 9, 2016

tiancaiamao Nov 9, 2016

coocood Nov 17, 2016 •

edited

Loading

zimulala commented Nov 17, 2016

tiancaiamao Nov 18, 2016 •

edited

Loading

zimulala Nov 21, 2016

tiancaiamao Nov 18, 2016

zimulala Nov 18, 2016

tiancaiamao Nov 18, 2016

zimulala Nov 18, 2016

tiancaiamao Nov 18, 2016

tiancaiamao Nov 18, 2016

zimulala Nov 18, 2016

coocood Nov 18, 2016

zimulala Nov 21, 2016

zimulala Nov 21, 2016

zimulala commented Nov 21, 2016

coocood commented Nov 21, 2016

tiancaiamao Nov 21, 2016

zimulala Nov 22, 2016

tiancaiamao commented Nov 22, 2016

zimulala commented Nov 22, 2016

		log.Errorf("[ddl] reload schema in loop err %v", errors.ErrorStack(err))
		time.Sleep(mustReloadSleepTime)

*: Reduce loading schema times #1984

*: Reduce loading schema times #1984

Conversation

zimulala commented Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

zimulala commented Nov 17, 2016

tiancaiamao Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Nov 21, 2016

coocood commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Nov 22, 2016

zimulala commented Nov 22, 2016

zimulala commented Nov 9, 2016 •

edited

Loading

coocood Nov 17, 2016 •

edited

Loading

tiancaiamao Nov 18, 2016 •

edited

Loading