Skip to content

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