[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #428: [YUNIKORN-1276] REST API for specific application

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

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


##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func getApplication(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	partition, partitionExists := vars["partition"]
+	if !partitionExists {
+		buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	queueName, queueNameExists := vars["queue"]
+	if !queueNameExists {
+		buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+
+	application, applicationExists := vars["application"]
+	if !applicationExists {
+		buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+
+	queueErr := validateQueue(queueName)
+	if queueErr != nil {
+		buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+		return
+	}
+	if len(vars) != 3 {
+		buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)

Review Comment:
   We need to be consistent with these checks. I would expect a standard ordering:
   1.  the number of arguments
   2. the existence of each argument by name
   3. the content of the argument syntax check (if required)
   
   We do not seem to do that consistently in any of the calls. It means we do too much processing before the eventual failure.



##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func getApplication(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	partition, partitionExists := vars["partition"]
+	if !partitionExists {
+		buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)

Review Comment:
   Need to make this constants or something not add the same text in multiple places in the handler, same for all error messages for parameter checks.



##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func getApplication(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	partition, partitionExists := vars["partition"]
+	if !partitionExists {
+		buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	queueName, queueNameExists := vars["queue"]
+	if !queueNameExists {
+		buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+
+	application, applicationExists := vars["application"]
+	if !applicationExists {
+		buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+
+	queueErr := validateQueue(queueName)
+	if queueErr != nil {
+		buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+		return
+	}
+	if len(vars) != 3 {
+		buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+
+	partitionContext := schedulerContext.GetPartitionWithoutClusterID(partition)
+	if partitionContext == nil {
+		buildJSONErrorResponse(w, "Partition not found", http.StatusBadRequest)
+		return
+	}
+
+	queue := partitionContext.GetQueue(queueName)
+	if queue == nil {
+		buildJSONErrorResponse(w, "Queue not found", http.StatusBadRequest)
+		return
+	}
+
+	app := queue.GetApplication(application)
+	if app == nil {
+		buildJSONErrorResponse(w, "Application not found", http.StatusBadRequest)
+		return
+	}
+
+	appDao := make([]*dao.ApplicationDAOInfo, 0, 1)

Review Comment:
   Do we really want to return an array of objects? We know we only have 1 application that we're returning.



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