docs: RFC: Enhancement of Internal System Session Management #60120
Conversation
ti-chi-bot
bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
size/L
lcwangchao
force-pushed
the
rfc_session_pool
branch
2 times, most recently
from
4aa4266 to
be483cc
Compare
lcwangchao
force-pushed
the
rfc_session_pool
branch
from
be483cc to
a0d1dfe
Compare
|
I don't know if the new version of the session pool will still use the implementation from github.com/ngaut/pools. However, I think this implementation is quite problematic and even less practical than the official pool. It's just that historically, there was a time when the Go language did not have sync.Pool , and implementing it ourselves had some advantages. |
|
According to the convention of |
Co-authored-by: djshow832 <[email protected]>
Thanks for the advise. I'll try to use the |
|
@lcwangchao: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
|
|
||
| func (s *Session) ExecuteInternal(ctx context.Context, sql string, args ...any) (sqlexec.RecordSet, error) { | ||
| if err := s.internal.checkOwner(s); err != nil { |
There was a problem hiding this comment.
instead of checkOwner, I think we can add a atomic bool closed field to *Session. When *Session is put back to the pool, store true to closed.
In other words, lifecycle management belongs to Session, not session
| if r := recover(); r != nil { | ||
| // Always destroy the session when panic to avoid undetermined state. | ||
| se.Destroy() | ||
| panic(r) | ||
| } | ||
| p.put(se) |
There was a problem hiding this comment.
I think if any error occurs, we should destroy it as well.
| } | ||
| ``` | ||
|
|
||
| The `mu` protects the internal session from concurrent access, and the field `sctx` is the real session context that holds the session's state. |
There was a problem hiding this comment.
I think we should discourage users from using the same session across multiple threads, as I’m concerned it could create a lot of issues. In the current session design, our expectation is that it shouldn’t be used this way.
There was a problem hiding this comment.
Or am I misunderstanding the purpose of this mutex?
What problem does this PR solve?
Issue Number: ref #60115
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.