[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #226: YUNIKORN-417: Add "nodes" REST API to fetch list of nodes

[GitBox <gi...@apache.org>]

yangwwei commented on a change in pull request #226:
URL: https://github.com/apache/incubator-yunikorn-core/pull/226#discussion_r562409403



##########
File path: pkg/webservice/handlers.go
##########
@@ -39,6 +39,8 @@ import (
 	"github.com/apache/incubator-yunikorn-core/pkg/scheduler/objects"
 	"github.com/apache/incubator-yunikorn-core/pkg/webservice/dao"
 	"github.com/apache/incubator-yunikorn-scheduler-interface/lib/go/si"
+
+	"github.com/gorilla/mux"

Review comment:
       hi @kingamarton  I think this license is fine, and actually, this dependency is already been used by YK. See more info from the dependency doc in https://cwiki.apache.org/confluence/display/INCUBATOR/YuniKornProposal. Close this as resolved.

##########
File path: pkg/webservice/handlers.go
##########
@@ -521,3 +523,26 @@ func updateConfiguration(conf string) (string, error) {
 	}
 	return "", fmt.Errorf("config plugin not found")
 }
+
+func getPartitionNodes(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	_, partitionExists := vars["partition"]
+	if len(vars) == 0 || !partitionExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	partition := schedulerContext.GetPartition(vars["partition"])

Review comment:
       Can we change line 530 to
   
   ```
   partitionName, partitionExists := vars["partition"]
   ```
   
   and here we can reuse the object:
   ```
   partition := schedulerContext.GetPartition(partitionName)
   ```

##########
File path: pkg/webservice/handlers.go
##########
@@ -521,3 +523,26 @@ func updateConfiguration(conf string) (string, error) {
 	}
 	return "", fmt.Errorf("config plugin not found")
 }
+
+func getPartitionNodes(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	_, partitionExists := vars["partition"]
+	if len(vars) == 0 || !partitionExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)

Review comment:
       Please explicitly include what parameter is missing in the error message.
   




----------------------------------------------------------------
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.

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