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

ddl: Revoke the session when the DDL will close (#3454) #3461

Merged
merged 3 commits into from Jun 12, 2017

Conversation

@zimulala
Copy link
Member

commented Jun 12, 2017

  • ddl: revoke the session and get env

Merge #3454 to master.

ddl: Revoke the session when the DDL will close (#3454)
* ddl: revoke the session and get env


* ddl: update

* ddl: address comments
@shenli

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

LGTM

if err != nil {
return errors.Trace(err)
}
ManagerSessionTTL = ttl

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 12, 2017

Contributor

In my opinion, use environment to control program's behavior is really a bad idea.
Why is this needed? is there any better way?

This comment has been minimized.

Copy link
@zimulala

zimulala Jun 12, 2017

Author Member

It's used for testing.

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 12, 2017

Contributor

But the real code is affected, if user specify a "manager_ttl=0" in the environment,
and get strange behavior, we have no way to know!

// If revoke takes longer than the ttl, lease is expired anyway.
ctx, cancel := goctx.WithTimeout(goctx.Background(),
time.Duration(ManagerSessionTTL)*time.Second)
_, err = m.etcdCli.Revoke(ctx, etcdSession.Lease())

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 12, 2017

Contributor

Why call with a timeout context, will Revoke would hang forever?

This comment has been minimized.

Copy link
@zimulala

zimulala Jun 12, 2017

Author Member

It revokes util its result is successful or failed, and etcd is so used.

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2017

LGTM

@zimulala zimulala merged commit 39b1fda into master Jun 12, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@zimulala zimulala deleted the zimuxia/ddl-session-revoke branch Jun 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.