Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: use async get timestamp API && remove asyncGetTSWorker #3459

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

tiancaiamao
Copy link
Contributor

With the new GetTSAsync API, a session doesn't need to bind a asyncGetTSWorker goroutine any more.

This change brings those benefits:

  1. Eliminate potential goroutine leak when session.Close is not called.
  2. Good for golang runtime scheduler as it eliminate a mass of background goroutines.

go asyncGetTSWorker(s.store, s.txnFutureCh)
func (tf *txnFuture) wait() (kv.Transaction, error) {
startTS, err := tf.future.Wait()
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to handle the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is always Canceled, caused by network timeout.
The real reason may be pd leader change or server down or network problem...

But it doesn't matter, if err not nil, we'll call store.Begin() in line 1070,
which would retry get timestamp until timeout.

txnFuture := s.getTxnFuture()
goCtx, cancelFunc := goctx.WithCancel(goctx.Background())
s.txnFuture, s.goCtx, s.cancelFunc = txnFuture, goCtx, cancelFunc
s.goCtx, s.cancelFunc = goctx.WithCancel(goctx.Background())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why store the Context inside a struct type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For historical reasons.
Each transaction goes along with a new context, transaction run in a session,
so the context is stored in the session.

@coocood
Copy link
Member

coocood commented Jun 13, 2017

LGTM

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 13, 2017
@shenli
Copy link
Member

shenli commented Jun 13, 2017

LGTM

@tiancaiamao tiancaiamao merged commit 821d928 into master Jun 13, 2017
@tiancaiamao tiancaiamao deleted the tiancaiamao/async-get-ts branch June 13, 2017 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants