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

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

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -610,3 +612,50 @@ func getPartitions(w http.ResponseWriter, r *http.Request) {
 		buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
 	}
 }
+
+func getQueueApplications(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	_, partitionExists := vars["partition"]
+	_, queueExists := vars["queue"]
+	var queueName = vars["queue"]
+
+	if !partitionExists {
+		buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if !queueExists {
+		buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+	}
+	if len(vars) != 2 {
+		buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	queueErr := validateQueue(queueName)
+	if queueErr != nil {
+		http.Error(w, queueErr.Error(), http.StatusBadRequest)
+		return
+	}
+	var appsDao []*dao.ApplicationDAOInfo
+	var partition = schedulerContext.GetPartition(vars["partition"])

Review comment:
       What is the value of `vars["partition"]` here?
   I think we should expect the partition name (without clusterID), e.g "default"
   To get this to work, I think there needs some more work here. I think we need to add a function to retrieve the partition with the short form name. E.g `GetPartitionWithoutClusterID()`. Right now, this function `schedulerContext.GetPartition()` seems to require the full name form: `[rmID]default`.
   could you please check this?

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -836,3 +838,89 @@ func TestMetricsNotEmpty(t *testing.T) {
 	handler.ServeHTTP(rr, req)
 	assert.Assert(t, len(rr.Body.Bytes()) > 0, "Metrics response should not be empty")
 }
+
+func TestGetQueueApplicationsHandler(t *testing.T) {
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	var err error
+	schedulerContext, err = scheduler.NewClusterContext(rmID, policyGroup)
+	assert.NilError(t, err, "Error when load clusterInfo from config")
+
+	assert.Equal(t, 1, len(schedulerContext.GetPartitionMapClone()))
+
+	// Check default partition
+	partitionName := "[" + rmID + "]default"
+	part := schedulerContext.GetPartition(partitionName)
+	assert.Equal(t, 0, len(part.GetApplications()))
+
+	// add a new app
+	app := newApplication("app-1", partitionName, "root.default", rmID)
+	err = part.AddApplication(app)
+	assert.NilError(t, err, "Failed to add Application to Partition.")
+	assert.Equal(t, app.CurrentState(), objects.New.String())
+	assert.Equal(t, 1, len(part.GetApplications()))
+
+	NewWebApp(schedulerContext, nil)
+
+	var req *http.Request
+	req, err = http.NewRequest("GET", "/ws/v1/partition/default/queue/root.default/applications", strings.NewReader(""))
+	vars := map[string]string{
+		"partition": "[rm-123]default",
+		"queue":     "root.default",
+	}

Review comment:
       Why the URL has partition: "default", but here in the vars we add "[rm-123]default" ?
   There is some inconsistency here. Could you please clarify?
   IMO, we should just use the partition name (without the clusterID) here, because clusterID is something internal.  

##########
File path: pkg/webservice/handlers.go
##########
@@ -610,3 +612,50 @@ func getPartitions(w http.ResponseWriter, r *http.Request) {
 		buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
 	}
 }
+
+func getQueueApplications(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	_, partitionExists := vars["partition"]
+	_, queueExists := vars["queue"]
+	var queueName = vars["queue"]
+
+	if !partitionExists {
+		buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if !queueExists {
+		buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+	}
+	if len(vars) != 2 {
+		buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	queueErr := validateQueue(queueName)
+	if queueErr != nil {
+		http.Error(w, queueErr.Error(), http.StatusBadRequest)

Review comment:
       why not using `buildJSONErrorResponse()` here?

##########
File path: pkg/webservice/handlers.go
##########
@@ -610,3 +612,50 @@ func getPartitions(w http.ResponseWriter, r *http.Request) {
 		buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
 	}
 }
+
+func getQueueApplications(w http.ResponseWriter, r *http.Request) {
+	vars := mux.Vars(r)
+	writeHeaders(w)
+	_, partitionExists := vars["partition"]
+	_, queueExists := vars["queue"]

Review comment:
       NIT: can we directly assign the variables here:
   ```
   partitionName, partitionExists := vars["partition"]
   queueName, queueExists := vars["queue"]
   ```

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -836,3 +838,89 @@ func TestMetricsNotEmpty(t *testing.T) {
 	handler.ServeHTTP(rr, req)
 	assert.Assert(t, len(rr.Body.Bytes()) > 0, "Metrics response should not be empty")
 }
+
+func TestGetQueueApplicationsHandler(t *testing.T) {
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	var err error
+	schedulerContext, err = scheduler.NewClusterContext(rmID, policyGroup)
+	assert.NilError(t, err, "Error when load clusterInfo from config")
+
+	assert.Equal(t, 1, len(schedulerContext.GetPartitionMapClone()))
+
+	// Check default partition
+	partitionName := "[" + rmID + "]default"

Review comment:
       we can leverage: `utils.GetNormalizedPartitionName`




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