-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spike: Use retry middleware with reverse proxy #64
Conversation
log.Printf("Proxying request to host %v: %v\n", host, id) | ||
newReverseProxy(host).ServeHTTP(w, proxyRequest) | ||
proxy := newReverseProxy(host) | ||
NewRetryMiddleware(h.retriesOnErr, proxy).ServeHTTP(w, proxyRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how the middleware is integrated with the reverse proxy
|
||
// NewDiscardableResponseWriter creates a new instance of the response writer delegator. | ||
// It takes a http.ResponseWriter and a function to determine by the status code if content should be written or discarded (for retry). | ||
func NewDiscardableResponseWriter(writer http.ResponseWriter, isDiscardable func(status int) bool) XResponseWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think wrapping the http.ResponseWriter comes with thorns. You can lose functionality if you don't implement all of the interfaces that ResponseWriter can be: https://go.dev/play/p/3B0yYC8kTvf
This is one the reasons I think that the middleware pattern in Go typically leads to more code than necessary. In the case of Lingo where we have a single primary handler (the proxy) I think that the middleware approach leads to more a more verbose/complicated solution than leaning on helper functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bringing benefits being customizable discussion here: #63
Closing this now in favour of #61 |
Same middleware as in #51 but set up for the reverse proxy