Throttle on HTTP throttle error

[jfudally]

Related issue: #832

Description

This PR changes the default throttling behavior when using an HTTP throttler (such as https://github.com/github/freno) that is unresponsive, or returns unexpected/missing data. This change throttles gh-ost during these failure scenarios. Because this changes the existing behavior, a --ignore-http-errors flag was added to allow the migration to continue if the HTTP throttler goes offline.

• contributed code is using same conventions as original code
• script/cibuild returns with no formatting errors, build errors or unit test errors.

[timvaillancourt]

@jfudally, I think this is pretty close 👍. I have 2 x suggestions:

collectFunc is called in a loop a 2nd-time in the collectThrottleHTTPStatus function:

gh-ost/go/logic/throttler.go

Lines 309 to 311 in df60fa4

	if sleep, _ := collectFunc(); sleep {
	time.Sleep(1 * time.Second)
	}

So we need to do the same error catching in the looped version too (or in collectFunc itself) or this will only catch an HTTP backend being down on the first attempt

Secondly, I think we should add an error message for the -1 HTTP code in these blocks here:

gh-ost/go/logic/throttler.go

Lines 21 to 37 in df60fa4

	var (
	httpStatusMessages map[int]string = map[int]string{
	200: "OK",
	404: "Not found",
	417: "Expectation failed",
	429: "Too many requests",
	500: "Internal server error",
	}
	// See https://github.com/github/freno/blob/master/doc/http.md
	httpStatusFrenoMessages map[int]string = map[int]string{
	200: "OK",
	404: "freno: unknown metric",
	417: "freno: access forbidden",
	429: "freno: threshold exceeded",
	500: "freno: internal error",
	}
	)

Without that the user will get a less-useful error message (http=-1) from the fallback here:

gh-ost/go/logic/throttler.go

Line 67 in df60fa4

return fmt.Sprintf("http=%d", statusCode)

[timvaillancourt]

Changes suggest in last reply

[jfudally]

@timvaillancourt Thanks for the review! I took care of these points you brought up. I did have some error logging, but it was pretty spammy once I caught the error out of the second call to collectFunc(), so I added an appropriate error message to the httpStatusMessages structs, which makes for a much cleaner message:

Copy: 0/0 100.0%; Applied: 0; Backlog: 0/1000; Time: 2s(total), 0s(copy); streamer: mysql-bin.000004:134210; Lag: 0.01s, State: throttled, Connection error (http=-1); ETA: due
Copy: 0/0 100.0%; Applied: 0; Backlog: 0/1000; Time: 3s(total), 0s(copy); streamer: mysql-bin.000004:138599; Lag: 0.01s, State: throttled, Connection error (http=-1); ETA: due
Copy: 0/0 100.0%; Applied: 0; Backlog: 0/1000; Time: 4s(total), 0s(copy); streamer: mysql-bin.000004:142993; Lag: 0.02s, State: throttled, Connection error (http=-1); ETA: due

[timvaillancourt]

@jfudally thanks 🎉, the latest changes LGTM!