-
Notifications
You must be signed in to change notification settings - Fork 407
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
pkg/server/api: Drop the muxer #242
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
b24d482
to
3c4fc99
Compare
Each sub-handler would be switching on Path anyway, and we don't have so many paths for the muxer's extra level of path switching to help much. And it's nice to only have to put things like [1]'s method guard in one spot (vs. repeating in each of the muxer's sub-handlers). [1]: openshift#240
3c4fc99
to
f5b7772
Compare
/hold |
LGTM |
IMO the muxer is how most people writer servers, i don't want to keep adding switches for package main
import (
"fmt"
"log"
"net/http"
)
type apiHandler struct{}
func (apiHandler) ServeHTTP(_ http.ResponseWriter, req *http.Request) {
log.Println(req.URL.Path)
}
func main() {
mux := http.NewServeMux()
mux.Handle("/healthz", apiHandler{})
mux.Handle("/config/", apiHandler{})
server := &http.Server{
Addr: fmt.Sprintf(":%v", 8080),
Handler: mux,
}
server.ListenAndServe()
} mux.go [9:56:55] ➜ playground go run mux.go
2018/12/17 10:01:49 /healthz
2018/12/17 10:02:25 /config/master curl [10:00:43] ➜ ~ curl http://localhost:8080/
404 page not found
[10:01:16] ➜ ~ curl http://localhost:8080/healthz
[10:02:06] ➜ ~ curl http://localhost:8080/config/master So i'm not sure what you mean by switching on Path in all handlers... ? |
So separate handler per path package main
import (
"fmt"
"log"
"net/http"
"path"
)
type healthzHandler struct{}
func (healthzHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
log.Println(req.URL.Path)
w.WriteHeader(http.StatusAccepted)
}
type configHandler struct{}
func (configHandler) ServeHTTP(_ http.ResponseWriter, req *http.Request) {
log.Println(req.URL.Path)
log.Println(path.Base(req.URL.Path))
}
func main() {
mux := http.NewServeMux()
mux.Handle("/healthz", healthzHandler{})
mux.Handle("/config/", configHandler{})
server := &http.Server{
Addr: fmt.Sprintf(":%v", 8080),
Handler: mux,
}
server.ListenAndServe()
} mux.go [10:10:11] ➜ playground go run mux.go
2018/12/17 10:10:21 /healthz
2018/12/17 10:10:27 /config/master
2018/12/17 10:10:27 master
2018/12/17 10:10:33 /config/worker
2018/12/17 10:10:33 worker curl [10:10:03] ➜ ~ curl -v http://localhost:8080/healthz
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /healthz HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.59.0
> Accept: */*
>
< HTTP/1.1 202 Accepted
< Date: Mon, 17 Dec 2018 18:10:21 GMT
< Content-Length: 0
<
* Connection #0 to host localhost left intact
[10:10:21] ➜ ~ curl -v http://localhost:8080/config/master
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /config/master HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.59.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Date: Mon, 17 Dec 2018 18:10:27 GMT
< Content-Length: 0
<
* Connection #0 to host localhost left intact
[10:10:27] ➜ ~ curl -v http://localhost:8080/config/worker
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /config/worker HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.59.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Date: Mon, 17 Dec 2018 18:10:33 GMT
< Content-Length: 0
<
* Connection #0 to host localhost left intact
[10:10:49] ➜ ~ |
Fair enough. It just comes down to how stable and complicated you expect this server to be. If you have lots of complicated, orthogonal routes, muxer makes a lot of sense. But if you only have |
keeping them in separate handlers allows the server to report more information in IMO so i want to keep the muxer, its not a very big overhead. |
/close |
@wking: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Each sub-handler would be switching on
Path
anyway, and we don't have so many paths for the muxer's extra level of path switching to help much. And it's nice to only have to put things like #240's method guard in one spot (vs. repeating in each of the muxer's sub-handlers).CC @abhinavdahiya