*: remove goCtx from session struct #5174
Conversation
1. go context should not be stored 2. change Executor interface to Open(goctx.Context) 3. many other changes forced by this refactor
|
tidb-test may also need update as a result of the change of some interfaces. |
ast/ast.go
Outdated
| @@ -182,7 +182,7 @@ type Statement interface { | |||
| OriginText() string | |||
|
|
|||
| // Exec executes SQL and gets a Recordset. | |||
| Exec(ctx context.Context) (RecordSet, error) | |||
| Exec(goCtx goctx.Context, ctx context.Context) (RecordSet, error) | |||
There was a problem hiding this comment.
Seems we don't need the second context.Context.
The Statement has one in its struct.
executor/adapter.go
Outdated
| @@ -167,7 +168,7 @@ func (a *ExecStmt) IsReadOnly() bool { | |||
| // This function builds an Executor from a plan. If the Executor doesn't return result, | |||
| // like the INSERT, UPDATE statements, it executes in this function, if the Executor returns | |||
| // result, execution is done after this function returns, in the returned ast.RecordSet Next method. | |||
| func (a *ExecStmt) Exec(ctx context.Context) (ast.RecordSet, error) { | |||
| func (a *ExecStmt) Exec(goCtx goctx.Context, ctx context.Context) (ast.RecordSet, error) { | |||
There was a problem hiding this comment.
The second ctx can be removed.
We can set ctx of ExecStmt at creation time.
executor/compiler.go
Outdated
| span1 := opentracing.StartSpan("executor.Compile", opentracing.ChildOf(span.Context())) | ||
| defer span1.Finish() | ||
| } | ||
| func (c *Compiler) Compile(goCtx goctx.Context, ctx context.Context, stmtNode ast.StmtNode) (*ExecStmt, error) { |
There was a problem hiding this comment.
We can remove the second ctx, add it to the field of Compiler.
session.go
Outdated
| // goCtx is used for cancelling the execution of current transaction. | ||
| goCtx goctx.Context | ||
|
|
||
| // cancelFunc is used for cancelling the execution of current transaction. | ||
| cancelFunc goctx.CancelFunc |
There was a problem hiding this comment.
The cancelFunc may be accessed by another goroutine. Need to be protected by mutex.
executor/join.go
Outdated
| @@ -198,8 +199,8 @@ func (e *HashJoinExec) fetchOuterRows() { | |||
| } | |||
|
|
|||
| select { | |||
| case <-e.ctx.GoCtx().Done(): | |||
| return | |||
| // case <-e.ctx.GoCtx().Done(): | |||
There was a problem hiding this comment.
Because ctx.GoCtx() method is removed, and we can't get a context to cancel the calculation.
As I've said
- some executor maybe not cancelable, because the break of context chain.
It will be fix in next PR soon, if Next() changed to Next(goctx.Context).
Comment the code rather than remove them, I can find it more easy next time.
There was a problem hiding this comment.
Because ctx.GoCtx() method is removed, and we can't get a context to cancel the calculation.
As I've said
- some executor maybe not cancelable, because the break of context chain.
It will be fix in next PR soon, if Next() changed to Next(goctx.Context).
Comment the code rather than remove them, I can find it more easy next time.
|
LGTM |
|
/run-all-tests |
zz-jason
added
status/LGT2
Some side effect introduced in this COMMIT:
I'll fix them in the following PRs.
@coocood