nsqd: configurable HTTP client transport timeout #776

kenjones-cisco · 2016-07-24T03:07:13Z

Adds configuration options HTTPClientConnectTimeout and
HTTPClientRequestTimeout to control the connection and request timeout
repectively of the HTTP client.

Also added to the following app binaries:

nsqadmin
nsq_stat
nsq_to_file
nsq_to_http

Closes #715
Closes #680

mreiferson · 2016-07-24T15:24:46Z

@kenjones-cisco thanks for this!

I have a high level comment first.

If we're going to land this, we should probably just do #680 as well, because the timeout as implemented is really a read/write deadline on the connection and has unexpected behavior.

If we do #680, we should introduce two configurable values, --http-client-connect-timeout and --http-client-request-timeout. They will apply to the custom Dial and to the HTTP client's Timeout property, respectively.

What do you think?

kenjones-cisco · 2016-07-24T15:31:45Z

@mreiferson That makes sense. Let me take a look.

mreiferson · 2016-07-24T17:03:59Z

Also, let's not forget to update nsqd's example config file with these options.

kenjones-cisco · 2016-07-24T17:50:44Z

@mreiferson changes made, just saw the comment about the example config file. Based on #715 I thought it was command line options. For nsqd and nsqadmin, the options should be specified for the configuration file as well?

mreiferson · 2016-07-24T17:56:19Z

For nsqd and nsqadmin, the options should be specified for the configuration file as well?

Might only be nsqd - it uses go-options to resolve configuration coming from multiple sources (the struct's defaults, command line flags, and config files).

mreiferson · 2016-07-24T17:58:22Z

internal/http_api/api_request.go

@@ -19,7 +19,6 @@ type deadlinedConn struct {
 }

 func (c *deadlinedConn) Read(b []byte) (n int, err error) {
-	c.Conn.SetReadDeadline(time.Now().Add(c.Timeout))


we should drop the one in Write too

kenjones-cisco · 2016-07-24T18:25:36Z

@mreiferson Looks like the DiskQueue test failure is back; otherwise all changes based on feedback are completed.

mreiferson · 2016-07-24T19:12:13Z

apps/nsq_stat/nsq_stat.go

@@ -27,6 +27,8 @@ var (
 	channel          = flag.String("channel", "", "NSQ channel")
 	statusEvery      = flag.Duration("status-every", -1, "(deprecated) duration of time between polling/printing output")
 	interval         = flag.Duration("interval", 2*time.Second, "duration of time between polling/printing output")
+	httpConnectTimeout = flag.Duration("http-client-connect-timeout", 2*time.Second, "timeout for HTTP connect")
+	httpRequestTimeout = flag.Duration("http-client-request-timeout", 2*time.Second, "timeout for HTTP request")


Didn't think of this before, but now that we're setting a real request timeout, which encapsulates connection time, we should set the default request timeout to something greater than connect, say 5s?

cc @jehiah

kenjones-cisco · 2016-07-25T00:16:18Z

@mreiferson Just let me know when a default is decided.

mreiferson · 2016-07-25T18:27:43Z

@kenjones-cisco let's go with 5s for now, @jehiah can chime in if he wants.

kenjones-cisco · 2016-07-26T02:28:40Z

@mreiferson all done

(also ran fmt.sh to make sure everything is properly formatted)

mreiferson · 2016-07-26T03:55:56Z

apps/nsq_to_http/nsq_to_http.go

-	httpTimeout   = flag.Duration("http-timeout", 20*time.Second, "timeout for HTTP connect/read/write (each)")
-	statusEvery   = flag.Int("status-every", 250, "the # of requests between logging status (per handler), 0 disables")
-	contentType   = flag.String("content-type", "application/octet-stream", "the Content-Type used for POST requests")
+	// deprecate? in favor of http-client-connect-timeout, http-client-request-timeout


Good catch, can you put the // TODO: prefix on this so it shows up in searches?

kenjones-cisco · 2016-07-27T02:44:35Z

@mreiferson Let me know if there is any more feedback.

mreiferson · 2016-07-27T15:12:32Z

apps/nsq_to_file/nsq_to_file.go

@@ -45,6 +45,9 @@ var (
 	rotateSize     = flag.Int64("rotate-size", 0, "rotate the file when it grows bigger than `rotate-size` bytes")
 	rotateInterval = flag.Duration("rotate-interval", 0*time.Second, "rotate the file every duration")

+	httpConnectTimeout = flag.Duration("http-client-connect-timeout", 5*time.Second, "timeout for HTTP connect")


should be 2s

mreiferson · 2016-07-27T15:14:36Z

@kenjones-cisco found a few more inconsistencies, thanks for your patience!

kenjones-cisco · 2016-07-27T15:17:05Z

Some were previously hard-coded to a specific value so I left those to avoid breaking existing functionality. But I can make the changes.

mreiferson · 2016-07-27T17:50:01Z

Some were previously hard-coded to a specific value so I left those to avoid breaking existing functionality. But I can make the changes.

@kenjones-cisco appreciate that, and I think we can still accomplish that by preserving those previously hard-coded values as the defaults for the flags, for those applications. What will be new (and potentially unexpected) is that we are now introducing a separate connection timeout in those cases. I believe 2s is a reasonable default.

Adds configuration options HTTPClientConnectTimeout and HTTPClientRequestTimeout to control the connection and request timeout repectively of the HTTP client. Also added to the following app binaries: - nsqadmin - nsq_stat - nsq_to_file - nsq_to_http Closes nsqio#715 Closes nsqio#680

kenjones-cisco · 2016-07-27T23:17:18Z

@mreiferson all changes completed

mreiferson · 2016-07-27T23:31:32Z

LGTM, thanks!

mreiferson added the feature label Jul 24, 2016

kenjones-cisco force-pushed the feature/http-client-timeout branch from de57ee8 to d6c0ce9 Compare July 24, 2016 17:44

mreiferson reviewed Jul 24, 2016
View reviewed changes

kenjones-cisco force-pushed the feature/http-client-timeout branch 2 times, most recently from 506cd97 to c422904 Compare July 24, 2016 18:21

mreiferson reviewed Jul 24, 2016
View reviewed changes

kenjones-cisco force-pushed the feature/http-client-timeout branch from c422904 to 7e8913f Compare July 26, 2016 02:24

mreiferson reviewed Jul 26, 2016
View reviewed changes

kenjones-cisco force-pushed the feature/http-client-timeout branch from 7e8913f to 2e07a9f Compare July 27, 2016 02:34

mreiferson reviewed Jul 27, 2016
View reviewed changes

kenjones-cisco force-pushed the feature/http-client-timeout branch from 2e07a9f to 454cf2f Compare July 27, 2016 22:53

mreiferson merged commit af2dfd5 into nsqio:master Jul 27, 2016

This was referenced Jul 27, 2016

nsqd: configurable HTTP client transport timeout #715

Closed

nsq_to_http: improve --http-timeout #680

Closed

kenjones-cisco deleted the feature/http-client-timeout branch July 28, 2016 20:45

jehiah mentioned this pull request Nov 2, 2016

nsqadmin: timeout settings #803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsqd: configurable HTTP client transport timeout #776

nsqd: configurable HTTP client transport timeout #776

kenjones-cisco commented Jul 24, 2016 •

edited

Loading

mreiferson commented Jul 24, 2016

kenjones-cisco commented Jul 24, 2016

mreiferson commented Jul 24, 2016

kenjones-cisco commented Jul 24, 2016

mreiferson commented Jul 24, 2016

mreiferson Jul 24, 2016

kenjones-cisco commented Jul 24, 2016

mreiferson Jul 24, 2016 •

edited

Loading

kenjones-cisco commented Jul 25, 2016

mreiferson commented Jul 25, 2016

kenjones-cisco commented Jul 26, 2016 •

edited

Loading

mreiferson Jul 26, 2016

kenjones-cisco commented Jul 27, 2016

mreiferson Jul 27, 2016

mreiferson commented Jul 27, 2016

kenjones-cisco commented Jul 27, 2016

mreiferson commented Jul 27, 2016

kenjones-cisco commented Jul 27, 2016

mreiferson commented Jul 27, 2016

nsqd: configurable HTTP client transport timeout #776

nsqd: configurable HTTP client transport timeout #776

Conversation

kenjones-cisco commented Jul 24, 2016 • edited Loading

mreiferson commented Jul 24, 2016

kenjones-cisco commented Jul 24, 2016

mreiferson commented Jul 24, 2016

kenjones-cisco commented Jul 24, 2016

mreiferson commented Jul 24, 2016

mreiferson Jul 24, 2016

Choose a reason for hiding this comment

kenjones-cisco commented Jul 24, 2016

mreiferson Jul 24, 2016 • edited Loading

Choose a reason for hiding this comment

kenjones-cisco commented Jul 25, 2016

mreiferson commented Jul 25, 2016

kenjones-cisco commented Jul 26, 2016 • edited Loading

mreiferson Jul 26, 2016

Choose a reason for hiding this comment

kenjones-cisco commented Jul 27, 2016

mreiferson Jul 27, 2016

Choose a reason for hiding this comment

mreiferson commented Jul 27, 2016

kenjones-cisco commented Jul 27, 2016

mreiferson commented Jul 27, 2016

kenjones-cisco commented Jul 27, 2016

mreiferson commented Jul 27, 2016

kenjones-cisco commented Jul 24, 2016 •

edited

Loading

mreiferson Jul 24, 2016 •

edited

Loading

kenjones-cisco commented Jul 26, 2016 •

edited

Loading