[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #530: [YUNIKORN-1472] Replacement for gorilla/mux

["wilfred-s (via GitHub)" <gi...@apache.org>]

wilfred-s commented on code in PR #530:
URL: https://github.com/apache/yunikorn-core/pull/530#discussion_r1184525607


##########
pkg/webservice/webservice.go:
##########
@@ -41,26 +42,29 @@ type WebService struct {
 	httpServer *http.Server
 }
 
-func newRouter() *mux.Router {
-	router := mux.NewRouter().StrictSlash(true)
+func newRouter() *httprouter.Router {
+	router := httprouter.New()
 	for _, webRoute := range webRoutes {
 		handler := loggingHandler(webRoute.HandlerFunc, webRoute.Name)
-		router.
-			Methods(webRoute.Method).
-			Path(webRoute.Pattern).
-			Name(webRoute.Name).
-			Handler(handler)
+		switch webRoute.Method {
+		case "GET":
+			router.HandlerFunc(http.MethodGet, webRoute.Pattern, handler)
+		case "PUT":
+			router.HandlerFunc(http.MethodPut, webRoute.Pattern, handler)
+		case "POST":
+			router.HandlerFunc(http.MethodPost, webRoute.Pattern, handler)
+		}

Review Comment:
   Unneeded switch, http.Method* are simple strings use:
   ```
   router.Handle(webroute.Method, webroute.Pattern, handler)
   ```



##########
pkg/webservice/webservice.go:
##########
@@ -41,26 +42,29 @@ type WebService struct {
 	httpServer *http.Server
 }
 
-func newRouter() *mux.Router {
-	router := mux.NewRouter().StrictSlash(true)
+func newRouter() *httprouter.Router {
+	router := httprouter.New()
 	for _, webRoute := range webRoutes {
 		handler := loggingHandler(webRoute.HandlerFunc, webRoute.Name)
-		router.
-			Methods(webRoute.Method).
-			Path(webRoute.Pattern).
-			Name(webRoute.Name).
-			Handler(handler)
+		switch webRoute.Method {
+		case "GET":
+			router.HandlerFunc(http.MethodGet, webRoute.Pattern, handler)
+		case "PUT":
+			router.HandlerFunc(http.MethodPut, webRoute.Pattern, handler)
+		case "POST":
+			router.HandlerFunc(http.MethodPost, webRoute.Pattern, handler)
+		}
 	}
 	return router
 }
 
-func loggingHandler(inner http.Handler, name string) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+func loggingHandler(inner http.Handler, name string) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
 		inner.ServeHTTP(w, r)
 		log.Logger().Debug(fmt.Sprintf("%s\t%s\t%s\t%s",
 			r.Method, r.RequestURI, name, time.Since(start)))

Review Comment:
   Remove the Sprintf. Building the string causes overhead we do not want/need. Move to standard logging call using zap.String(name, value) for each, ordering: name, method, URI and time.Since(start)



##########
pkg/webservice/handlers.go:
##########
@@ -481,9 +482,9 @@ func getPartitions(w http.ResponseWriter, r *http.Request) {
 }
 
 func getPartitionQueues(w http.ResponseWriter, r *http.Request) {
-	vars := mux.Vars(r)
+	vars := httprouter.ParamsFromContext(r.Context())

Review Comment:
   httprouter.ParamsFromContext can return nil: https://github.com/julienschmidt/httprouter/blob/master/router.go#L118-L119
   Should we not handle that or are we 100% sure that can never happen?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org