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

etcdctl: use a context with -total-timeout in simple commands #3611

Merged
merged 1 commit into from
Oct 14, 2015

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Sep 29, 2015

Like the commit 8ebc933, this commit lets simple etcdctl commands
use a context with timeout value passed via -total-timeout.

This commit doesn't change complex commands like watch,
cluster-health, and import because it is not obvious that using the
context in the commands is good or not.

Like the commit 8ebc933, this commit lets simple etcdctl commands
use a context with timeout value passed via -total-timeout.

This commit doesn't change complex commands like watch,
cluster-health, and import because it is not obvious that using the
context in the commands is good or not.
@yichengq
Copy link
Contributor

LGTM. defer to @xiang90

@mitake
Copy link
Contributor Author

mitake commented Oct 6, 2015

It seems that semaphoreci is causing problems:

  • timeout of connection
  • datarace caused by time.AfterFunc()'s goroutine

The latter problem seems to be solved in de1a16e . The first one would be caused by too short timeout value. Should I enlarge the timeout value for solving it?

@yichengq
Copy link
Contributor

yichengq commented Oct 7, 2015

timeout of connection

I cannot ensure that the problem is caused by short timeout, and the reason that semaphoreci easily fails on this one while travis doesn't.

@mitake
Copy link
Contributor Author

mitake commented Oct 14, 2015

@yichengq then can I conclude that the problem is caused by semaphoreci not by my PR?

@yichengq
Copy link
Contributor

@mitake Yes. It will pass if i rebuild it.

@yichengq
Copy link
Contributor

LGTM again. Thanks for totally fixing the timeout flag!

yichengq added a commit that referenced this pull request Oct 14, 2015
etcdctl: use a context with -total-timeout in simple commands
@yichengq yichengq merged commit afd74df into etcd-io:master Oct 14, 2015
@mitake
Copy link
Contributor Author

mitake commented Oct 15, 2015

@yichengq thanks for your review and merge!

@mitake mitake deleted the etcdctl-timeout branch December 30, 2015 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants