[GitHub] [yunikorn-core] manirajv06 commented on a diff in pull request #465: [YUNIKORN-1439] expose user and group endpoints

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

manirajv06 commented on code in PR #465:
URL: https://github.com/apache/yunikorn-core/pull/465#discussion_r1040534325


##########
pkg/scheduler/ugm/manager.go:
##########
@@ -221,6 +221,15 @@ func (m *Manager) GetUsersResources() []*UserTracker {
 	return userTrackers
 }
 
+func (m *Manager) GetUserResourcesByUserName(user string) *UserTracker {

Review Comment:
   Can we rename the method name as GetUserResources?



##########
pkg/scheduler/ugm/manager.go:
##########
@@ -231,6 +240,15 @@ func (m *Manager) GetGroupsResources() []*GroupTracker {
 	return groupTrackers
 }
 
+func (m *Manager) GetGroupResourcesByGroupName(group string) *GroupTracker {

Review Comment:
   Same like earlier



##########
pkg/webservice/handlers_test.go:
##########
@@ -1358,6 +1376,34 @@ func TestUsersAndGroupsResourceUsage(t *testing.T) {
 	assert.Equal(t, usersResourceUsageDao[0].Queues.ResourceUsage.String(),
 		resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.CPU: 1}).String())
 
+	// Test existed user query
+	req, err = http.NewRequest("GET", "/ws/v1/partition/default/usage/user/testuser", strings.NewReader(""))
+	vars := map[string]string{
+		"user":  "testuser",
+		"group": "testgroup",
+	}
+	req = mux.SetURLVars(req, vars)
+	assert.NilError(t, err, "Get User Resource Usage Handler request failed")
+	resp = &MockResponseWriter{}
+	var userResourceUsageDao *dao.UserResourceUsageDAOInfo
+	getUserResourceUsage(resp, req)
+	err = json.Unmarshal(resp.outputBytes, &userResourceUsageDao)
+	assert.NilError(t, err, "failed to unmarshal user resource usage dao response from response body: %s", string(resp.outputBytes))
+	assert.Equal(t, userResourceUsageDao.Queues.ResourceUsage.String(),
+		resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.CPU: 1}).String())
+
+	// Test non-existing user query
+	req, err = http.NewRequest("GET", "/ws/v1/partition/default/usage/user/testNonExistingUser", strings.NewReader(""))
+	vars = map[string]string{
+		"user":  "testNonExistingUser",
+		"group": "testgroup",
+	}
+	req = mux.SetURLVars(req, vars)
+	assert.NilError(t, err, "Get User Resource Usage Handler request failed")
+	resp = &MockResponseWriter{}
+	getUserResourceUsage(resp, req)
+	assertUserExists(t, resp)
+

Review Comment:
   Can we have a separate test method for this handler (may be include below one as well) unit tests to avoid clubbing multiple handlers in same method?



##########
pkg/webservice/handlers_test.go:
##########
@@ -1358,6 +1376,34 @@ func TestUsersAndGroupsResourceUsage(t *testing.T) {
 	assert.Equal(t, usersResourceUsageDao[0].Queues.ResourceUsage.String(),
 		resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.CPU: 1}).String())
 
+	// Test existed user query
+	req, err = http.NewRequest("GET", "/ws/v1/partition/default/usage/user/testuser", strings.NewReader(""))

Review Comment:
   Is there a way to assert " /ws/v1/partition/default/usage/user/ " or without specific user or group name being passed? In this case, there is no need to call the corresponding method in ugm module and "user or group name is missing or mandatory param is missing" 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.

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

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