etcdctlv3: add dial timeout flag #4838
Conversation
|
Added e2e tests as well. It has lots of Please review. Thanks. |
|
|
||
| func setupEtcdctlV3Test(t *testing.T, cfg *etcdProcessClusterConfig, quorum bool) *etcdProcessCluster { | ||
| // TODO: add serialized option | ||
| quorum = true // now linearlized read by default |
There was a problem hiding this comment.
why override quorum here? it doesn't make sense for some commands (e.g., defrag)
There was a problem hiding this comment.
right. Will remove. Won't have any effect on current v3 ctl anyway.
Thanks.
| @@ -56,11 +56,15 @@ func makeMirrorCommandFunc(cmd *cobra.Command, args []string) { | |||
| if len(args) != 1 { | |||
| ExitWithError(ExitBadArgs, errors.New("make-mirror takes one destination arguement.")) | |||
| } | |||
| dialTimeout, err := cmd.Flags().GetDuration("dial-timeout") | |||
There was a problem hiding this comment.
replace this (and the other one) with a call to dialTimeoutFromCmd(cmd) in global.go?
| @@ -46,6 +47,8 @@ func init() { | |||
| rootCmd.PersistentFlags().StringVarP(&globalFlags.OutputFormat, "write-out", "w", "simple", "set the output format (simple, json, protobuf)") | |||
| rootCmd.PersistentFlags().BoolVar(&globalFlags.IsHex, "hex", false, "print byte strings as hex encoded strings") | |||
|
|
|||
| rootCmd.PersistentFlags().DurationVar(&globalFlags.DialTimeout, "dial-timeout", 5*time.Second, "dial timeout for API connections") | |||
There was a problem hiding this comment.
2 seconds default value seems to be more than enough.
| // checkTesting(t, testCtlV3Set(t, &configTLS, 5*time.Second, true)) | ||
| // } | ||
|
|
||
| func checkTesting(t *testing.T, err error) { |
There was a problem hiding this comment.
I don't think this function is necessary. The test functions are already getting a testing.T so they can Fatalf instead of returning errors.
There was a problem hiding this comment.
I had it for testing timeout. Should I just add another arg to check the timeout error?
There was a problem hiding this comment.
actually, it will be cleaner without this function. Will change.
There was a problem hiding this comment.
shouldn't need another argument-- testCtlV3Set could accept the timeout error when the dialtimeout is a nanosecond
There was a problem hiding this comment.
right. will fix. thanks.
| testCtlV3Set(t, &configNoTLS, time.Nanosecond, true, true) | ||
| } | ||
|
|
||
| // TODO: Getting transport: x509: certificate signed by unknown authority |
There was a problem hiding this comment.
please remove commented out code
| } | ||
| } | ||
|
|
||
| func etcdctlV3PrefixArgs(clus *etcdProcessCluster, dialTimeout time.Duration) []string { |
There was a problem hiding this comment.
can we remove all etcd mentioning in this file? it is clearly etcd specific test.
There was a problem hiding this comment.
Agree. Just cleaned up the test functions in this file, but not the others ones in other tests. We can change later?
Thanks.
|
LGTM. defer to @heyitsanthony |
|
lgtm |
|
thanks. will merge after I figure out why |
gyuho
force-pushed
the
dial
branch
3 times, most recently
from
5aae546
to
bd33130
Compare
|
Was manipulating the default configs by pointer. Now tests pass. |
etcdctlv3: add dial timeout flag
Fix #4836.