Skip to content

Throttle on HTTP throttle error#833

Merged
jfudally merged 11 commits intomasterfrom
jfudally-throttle-fix
Mar 30, 2020
Merged

Throttle on HTTP throttle error#833
jfudally merged 11 commits intomasterfrom
jfudally-throttle-fix

Conversation

@jfudally
Copy link

@jfudally jfudally commented Mar 25, 2020 •
edited
Loading

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.

@jfudally jfudally requested a review from a team March 25, 2020 19:41
@jfudally jfudally added the bug label Mar 25, 2020
@jfudally jfudally self-assigned this Mar 25, 2020
@jfudally jfudally added the wip label Mar 25, 2020
@jfudally jfudally removed the wip label Mar 27, 2020
@timvaillancourt
Copy link
Collaborator

timvaillancourt commented Mar 27, 2020 •
edited
Loading

@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
timvaillancourt requested changes Mar 27, 2020
View reviewed changes
Copy link
Collaborator

@timvaillancourt timvaillancourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes suggest in last reply

@jfudally
Copy link
Author

jfudally commented Mar 30, 2020

@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
Copy link
Collaborator

timvaillancourt commented Mar 30, 2020

@jfudally thanks 🎉, the latest changes LGTM!

@jfudally jfudally temporarily deployed to staging March 30, 2020 16:38 Inactive
@jfudally jfudally temporarily deployed to staging March 30, 2020 17:27 Inactive
@jfudally jfudally temporarily deployed to production March 30, 2020 19:32 Inactive
@jfudally jfudally merged commit 4dab06e into master Mar 30, 2020
@jfudally jfudally deleted the jfudally-throttle-fix branch March 30, 2020 19:39
@jfudally jfudally mentioned this pull request Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timvaillancourt timvaillancourt timvaillancourt approved these changes

Assignees

@jfudally jfudally

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jfudally @timvaillancourt