executor: IndexLookUpExecutor refactor #4305

tiancaiamao · 2017-08-23T13:18:27Z

When the executor close, all background goroutines should exit
Objects should maintain smaller inner state, high cohesion and low coupling
Avoid data race and make the code more reasonable

shenli · 2017-08-24T01:35:14Z

executor/distsql.go

-	cursor  int
-	done    bool
-	doneCh  chan error
+	// if tasksErr is not nil, this is a invalid lookupTableTask.


Can not understand the comment.

indexHandler send lookupTableTask to a channel, then
tableHandler receive lookupTableTask from channel and further handle.

They communicate through channel, but what happens when indexHandler meet error?
it has to pass that information, so that's why there is a tasksErr field.
@shenli

shenli · 2017-08-24T01:36:31Z

executor/new_distsql.go

 	finished chan struct{}
 }

+type indexHandler struct {


Add comment for the struct.

shenli · 2017-08-24T01:38:51Z

executor/new_distsql.go

+	ih.wg.Wait()
+}
+
+type tableHandler struct {


zimulala · 2017-08-24T10:27:28Z

executor/new_distsql.go

+}
+
+func (ih *indexHandler) Open(kvRanges []kv.KeyRange, e *IndexLookUpExecutor, workCh chan<- *lookupTableTask, finished <-chan struct{}) error {
+	result, err := distsql.SelectDAG(e.ctx.GetClient(), e.ctx.GoCtx(), e.dagPB, kvRanges, e.ctx.GetSessionVars().DistSQLScanConcurrency, e.keepOrder, e.desc, getIsolationLevel(e.ctx.GetSessionVars()), e.priority)


This line too long ...

zimulala · 2017-08-24T11:07:13Z

executor/new_distsql.go

+		ih.fetchHandles(result, workCh, ctx, finished)
+		cancel()
+		if err := result.Close(); err != nil {
+			log.Errorf(errors.ErrorStack(err))


Complete this log.

alivxxx · 2017-08-28T03:19:26Z

executor/new_distsql.go

+	ih.wg.Wait()
+}
+
+// indexHandler is used by IndexLookUpExecutor to maintain table lookup background goroutines.


Should be tableHandler？

shenli · 2017-08-28T03:47:41Z

executor/new_distsql.go

+	executeTask func(task *lookupTableTask, goCtx goctx.Context)
+}
+
+func (th *tableHandler) Open(e *IndexLookUpExecutor, workCh <-chan *lookupTableTask, finished <-chan struct{}) {


Can we change Open to open?

Current Executor interface need Open method.

shenli · 2017-08-28T08:08:50Z

LGTM

shenli · 2017-08-28T13:13:15Z

@hanfei1991 @lamxTyler PTAL

coocood · 2017-09-04T02:08:21Z

executor/new_distsql.go

+	}
+	return indexRangesToKVRanges(e.ctx.GetSessionVars().StmtCtx, e.tableID, e.index.ID, e.ranges, fieldTypes)
+}
+
 // doRequestForDatums constructs kv ranges by datums. It is used by index look up executor.


Add comment about implements DataReader interface.
And it is used by index look up join instead of index lookup executor

coocood · 2017-09-04T02:52:28Z

executor/new_distsql.go

+	e.indexHandler = indexHandler{}
+	e.tableHandler = tableHandler{}
+
+	// workCh serves as a pipeline, indexHandler will write to workCh and tableHandler


workCh is not a pipeline, pipeline means one in one out, but workCh is one in many out.

coocood · 2017-09-04T03:19:40Z

executor/new_distsql.go

+				th.taskChan <- task
+			} else {
+				th.taskChan <- task
+				return


Do we really need to return here?
When error occurred in fetchHandles, the workCh will be closed soon.

This can be simpler.

if task.tasksErr == nil { th.executeTask(task, ctx) } th.taskChan <- task

If error happened, we should return.

There are too many returns here, makes it hard to reason about.
If we remove the redundant returns, we can infer the sequence of execution easier.

And our code style is to handle error first, this looks awkward.

coocood · 2017-09-04T03:42:03Z

executor/new_distsql.go

 	finished chan struct{}
 }

+// indexHandler is used by IndexLookUpExecutor to maintain index lookup background goroutines.
+type indexHandler struct {
+	buildTableTasks func(handles []int64) []*lookupTableTask


I think variable function makes the code more complex.
How about just hold a reference to IndexLookUpExecutor in indexHandler?
So we can just implement buildTableTasks directly in indexHandler.

I've consider that at the first time but abandon the idea.
IndexLookUpExecutor include indexHandler while indexHandler hold a reference to IndexLookUpExecutor, it make the code even more clumsy.
As long as the the data fields buildTableTasks visited is all immutable, I think using function pointer is affordable.

This is a very common pattern, a member holds a reference to its parent.
Except that when the member is embedded.
I think it's better if you make indexHandler and tableHandler an instance instead of embedded member.

Another solution is to add prefix to the functions, like indexOpen, tableOpen, indexFetchHandles.
So we don't need to define indexHandler and tableHandler.

Use embedded member to separate the logic is not straightforward.

In the object-oriented style, we don't care the inner state of the object, just call the methods provide by it. That's a good abstraction for here, I don't want IndexLookUpExecutor to cope with a huge couple of inner state, so I split them into smaller object.

I don't remember I saw any Go code like this, assign the parent's method to embedded member's function field.
It's like use a very rare way to solve a common problem.

coocood · 2017-09-04T03:43:58Z

executor/new_distsql.go

+type tableHandler struct {
+	taskChan    chan *lookupTableTask
+	taskCurr    *lookupTableTask
+	executeTask func(task *lookupTableTask, goCtx goctx.Context)


coocood · 2017-09-05T02:00:24Z

executor/new_distsql.go

+	}()
+}
+
+func (th *tableHandler) pickAndExecTask(workCh <-chan *lookupTableTask, ctx goctx.Context, finished <-chan struct{}) {


It's better to add comment on a background worker function.

coocood · 2017-09-05T02:02:54Z

executor/new_distsql.go

+	executeTask func(task *lookupTableTask, goCtx goctx.Context)
+}
+
+func (th *tableHandler) open(e *IndexLookUpExecutor, workCh <-chan *lookupTableTask, finished <-chan struct{}) {


Add comment for the function.

coocood · 2017-09-05T02:06:45Z

executor/new_distsql.go

+	wg              sync.WaitGroup
+}
+
+func (ih *indexHandler) open(kvRanges []kv.KeyRange, e *IndexLookUpExecutor, workCh chan<- *lookupTableTask, finished <-chan struct{}) error {


Need comments about what does this function do.

tiancaiamao · 2017-09-05T06:07:13Z

PTAL @coocood

coocood · 2017-09-05T06:17:36Z

executor/new_distsql.go

+	wg sync.WaitGroup
+}
+
+// openIndexHandler open the indexHandler, launch some background worker goroutines and write the results to workCh.


It only launch one background goroutine to fetch handles.
send the results to workCh.

coocood · 2017-09-05T06:25:21Z

executor/new_distsql.go

+	}
+}
+
+func (th *tableHandler) next() (Row, error) {


I think move this method to IndexLookUpExecutor is better, it's a foreground function, tableHandler is supposed to handle background work.

coocood · 2017-09-05T06:29:43Z

executor/new_distsql.go

+	}
+}
+
+func (ih *indexHandler) close() {


func (e *IndexLookUpExecutor) closeIndexHandler

For symmetricity.

coocood · 2017-09-05T06:29:52Z

executor/new_distsql.go

+	}
+}
+
+func (th *tableHandler) close() {


coocood · 2017-09-05T07:55:03Z

executor/new_distsql.go

 	for {
-		if e.taskCurr == nil {
-			taskCurr, ok := <-e.taskChan
+		if th.taskCurr == nil {


move taskCurr to IndexLookUpExecutor is better.

I move next() function back to tableHandler.
taskCurr is tableHandler's inner state, IndexLookUpExecutor just call method provided by tableHandler, never touch its inner state.

coocood · 2017-09-05T07:55:15Z

executor/new_distsql.go

+	}
+}
+
+func (ih *indexHandler) closeIndexHandler() {


The method receiver should be IndexLookUpExecutor.

This reminds me func (ih *indexHandler) close() is better.

func (ih *indexHandler) close() { ih.wg.Wait() }

func (e *IndexLookUpExecutor) closeIndexHandler() { // e visit indexHandler's inner state, that's bad. e.indexHandler.wg.Wait() }

// From the previous view, this is also asymmetric, isn't it? xxx := NewXXX() xxx.Close()

coocood · 2017-09-05T07:55:40Z

executor/new_distsql.go

+	}
+}
+
+func (th *tableHandler) closeTableHandler() {


coocood · 2017-09-05T07:58:08Z

executor/new_distsql.go

+	th := &e.tableHandler
+	th.taskChan = make(chan *lookupTableTask, atomic.LoadInt32(&LookupTableTaskChannelSize))
+	lookupConcurrencyLimit := e.ctx.GetSessionVars().IndexLookupConcurrency
+	var wg sync.WaitGroup


How about make wg as a member of tableHandler like indexHandler?

This variable is not necessary to be the inner state of tableHandler, a local variable is enough.
tableHandler close doesn't wait on it.

shenli · 2017-09-06T01:53:44Z

LGTM

shenli · 2017-09-06T01:53:51Z

/run-all-test

coocood · 2017-09-06T01:50:51Z

executor/new_distsql.go

+
+// tableWorker is used by IndexLookUpExecutor to maintain table lookup background goroutines.
+type tableWorker struct {
+	taskChan chan<- *lookupTableTask


I think resultChan maybe a better name. task is the type of the channel, it doesn't give any information.
the table worker pick a task from workChan, after finished, send the task to resultChan, this is easier to understand.

coocood · 2017-09-06T03:01:56Z

executor/new_distsql.go

 	finished chan struct{}
+
+	taskChan <-chan *lookupTableTask


coocood · 2017-09-06T03:02:14Z

executor/new_distsql.go

 	finished chan struct{}
+
+	taskChan <-chan *lookupTableTask
+	taskCurr *lookupTableTask


tiancaiamao · 2017-09-06T03:12:14Z

/run-all-test

tiancaiamao · 2017-09-06T05:50:49Z

PTAL @coocood

coocood · 2017-09-06T07:34:59Z

LGTM

executor: IndexLookUpExecutor refactor

0a92486

shenli reviewed Aug 24, 2017

View reviewed changes

tiancaiamao added 2 commits August 24, 2017 11:31

tiny update

189922f

address comment

57cdf69

tiancaiamao changed the title ~~[WIP] executor: IndexLookUpExecutor refactor~~ executor: IndexLookUpExecutor refactor Aug 24, 2017

tiancaiamao added the priority/P2 label Aug 24, 2017

zimulala reviewed Aug 24, 2017

View reviewed changes

tiancaiamao added 2 commits August 24, 2017 19:21

address comment

4d2ceac

fix race

e3064bf

tiancaiamao force-pushed the tiancaiamao/refactor-indexlookup branch from 379f2de to e3064bf Compare August 24, 2017 11:58

tiancaiamao added 2 commits August 24, 2017 23:48

Merge branch 'master' into refactor-indexlookup

5a306de

make golint happy

2eba1c8

alivxxx reviewed Aug 28, 2017

View reviewed changes

fix typo

e9387e1

shenli reviewed Aug 28, 2017

View reviewed changes

tiancaiamao added 2 commits August 28, 2017 13:28

change some function to lower case

3f442cd

Merge branch 'master' into refactor-indexlookup

bf10031

zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 29, 2017

coocood reviewed Sep 4, 2017

View reviewed changes

address comment

e1519eb

tiancaiamao force-pushed the tiancaiamao/refactor-indexlookup branch from f1cea0d to e1519eb Compare September 4, 2017 13:39

address comment

f23e4be

coocood reviewed Sep 5, 2017

View reviewed changes

tiancaiamao added 2 commits September 5, 2017 11:51

address comment

a77a9a7

address comment

9c5d717

coocood reviewed Sep 5, 2017

View reviewed changes

address comment

3c4ffe8

coocood reviewed Sep 5, 2017

View reviewed changes

tiancaiamao added 2 commits September 5, 2017 19:24

address comment

bdbcc20

address commet

e974bbe

coocood reviewed Sep 6, 2017

View reviewed changes

shenli previously approved these changes Sep 6, 2017

View reviewed changes

tiancaiamao added 2 commits September 6, 2017 10:14

address comment

f4ae018

Merge branch 'master' into refactor-indexlookup

efb1b0b

tiancaiamao dismissed shenli’s stale review via efb1b0b September 6, 2017 02:16

tiancaiamao force-pushed the tiancaiamao/refactor-indexlookup branch from 7a918a7 to efb1b0b Compare September 6, 2017 02:16

coocood reviewed Sep 6, 2017

View reviewed changes

address comment

ca2890e

Merge branch 'master' into tiancaiamao/refactor-indexlookup

8dd43cd

Merge branch 'master' into tiancaiamao/refactor-indexlookup

e274169

zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 6, 2017

zimulala approved these changes Sep 6, 2017

View reviewed changes

tiancaiamao merged commit dba58e9 into master Sep 6, 2017

tiancaiamao deleted the tiancaiamao/refactor-indexlookup branch September 6, 2017 08:45

executor: IndexLookUpExecutor refactor #4305

executor: IndexLookUpExecutor refactor #4305

Conversation

tiancaiamao commented Aug 23, 2017

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

shenli commented Aug 28, 2017

shenli commented Aug 28, 2017

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

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

tiancaiamao commented Sep 5, 2017

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

shenli commented Sep 6, 2017

shenli commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Sep 6, 2017

tiancaiamao commented Sep 6, 2017

coocood commented Sep 6, 2017