[YUNIKORN-2190] Using empty array to replace null response from /ws/v1/partition/{partitionName}/usage/users #755
Conversation
brandboat
force-pushed
the
YUNIKORN-2190
branch
from
d91ea3a
to
e9cf94a
Compare
There was a problem hiding this comment.
@brandboat thanks for this patch. a couple of comments below.
pkg/webservice/handlers.go
Outdated
| @@ -823,7 +823,7 @@ func getUsersResourceUsage(w http.ResponseWriter, _ *http.Request) { | |||
| writeHeaders(w) | |||
| userManager := ugm.GetUserManager() | |||
| usersResources := userManager.GetUsersResources() | |||
| var result []*dao.UserResourceUsageDAOInfo | |||
| result := []*dao.UserResourceUsageDAOInfo{} | |||
There was a problem hiding this comment.
how about using make with initial capacity?
result := make([]*dao.UserResourceUsageDAOInfo, len(usersResources))
There was a problem hiding this comment.
Actually I think it has to be result := make([]*dao.UserResourceUsageDAOInfo, 0, len(usersResources)) so that appending starts at position 0.
There was a problem hiding this comment.
The even faster solution is to use this:
result := make([]*dao.UserResourceUsageDAOInfo, len(usersResources))
for i, tracker := range usersResources {
result[i] = tracker.GetUserResourceUsageDAOInfo()
}
There was a problem hiding this comment.
Already addressed comments, thank you for everyone's valuable suggestions !
pkg/webservice/handlers.go
Outdated
| @@ -859,7 +859,7 @@ func getGroupsResourceUsage(w http.ResponseWriter, r *http.Request) { | |||
| writeHeaders(w) | |||
| userManager := ugm.GetUserManager() | |||
| groupsResources := userManager.GetGroupsResources() | |||
| var result []*dao.GroupResourceUsageDAOInfo | |||
| result := []*dao.GroupResourceUsageDAOInfo{} | |||
brandboat
force-pushed
the
YUNIKORN-2190
branch
from
e9cf94a
to
ef0f2c1
Compare
pkg/webservice/handlers_test.go
Outdated
| @@ -1500,7 +1500,7 @@ func TestUsersAndGroupsResourceUsage(t *testing.T) { | |||
| assert.NilError(t, err, "Get Groups Resource Usage Handler request failed") | |||
|
|
|||
| var groupsResourceUsageDao []*dao.GroupResourceUsageDAOInfo | |||
| var expGroupsResourceUsageDao []*dao.GroupResourceUsageDAOInfo | |||
| expGroupsResourceUsageDao := []*dao.GroupResourceUsageDAOInfo{} | |||
There was a problem hiding this comment.
pardon me, is this change related to PR? or it is a potential issue?
There was a problem hiding this comment.
Yes it's related. Since the response from the code you mentioned is now empty array instead of nil as we changed the behavior in this pr.
IMO, the test should be adjusted as follows: set up a scenario where the group resource usage is not empty, ensuring that there is content in groupsResourceUsageDao. Otherwise, it appears to be duplicating the functionality of TestEmptyGroupsResourceUsageHandler.
There was a problem hiding this comment.
IMO, the test should be adjusted as follows: set up a scenario where the group resource usage is not empty, ensuring that there is content in groupsResourceUsageDao. Otherwise, it appears to be duplicating the functionality of TestEmptyGroupsResourceUsageHandler.
that makes sense to me. Please make sure the test is NOT duplicate to TestEmptyGroupsResourceUsageHandler
There was a problem hiding this comment.
@brandboat Empty response is one of the basic test cases while hitting those API's. So, can we add the test related changes in TestUsersAndGroupsResourceUsage itself instead of two new & separate test methods?
There was a problem hiding this comment.
Already addressed comments in the latest commit, many thanks !
…1/partition/{partitionName}/usage/users
also make the same change to /ws/v1/partition/{partitionName}/usage/groups
brandboat
force-pushed
the
YUNIKORN-2190
branch
from
ef0f2c1
to
aaafc9c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #755 +/- ##
==========================================
+ Coverage 77.72% 77.75% +0.02%
==========================================
Files 82 82
Lines 13430 13430
==========================================
+ Hits 10439 10442 +3
+ Misses 2664 2662 -2
+ Partials 327 326 -1 ☔ View full report in Codecov by Sentry. |
| @@ -1405,7 +1422,7 @@ func TestFullStateDumpPath(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestSpecificUserAndGroupResourceUsage(t *testing.T) { | |||
| prepareUserAndGroupContext(t) | |||
| prepareUserAndGroupContext(t, configDefault) | |||
There was a problem hiding this comment.
(this is not related to this PR)
it seems the configDefault does not include any group/user, so all asserts in this test case are related to "nonexistent" case, right?
There was a problem hiding this comment.
it seems the configDefault does not include any group/user, so all asserts in this test case are related to "nonexistent" case, right?
You're correct, except for the test case under comment // Test existed user query. I believe the test case mentioned in that comment is missing an assertion to verify the existence of the user resource testuser.
There was a problem hiding this comment.
That is unrelated to this PR, so I file ticket (https://issues.apache.org/jira/browse/YUNIKORN-2272) to trace it.
There was a problem hiding this comment.
Thanks, already take over the ticket, will come up with another pr ASAP.
What is this PR for?
The endpoint
/ws/v1/partition/{partitionName}/usage/usersand/ws/v1/partition/{partitionName}/usage/groupswill return nil when user / group usages are nonexistent. The return type is expected to be array, so it seems to me the empty array is more suitable to be returned.
What type of PR is it?
Todos
N/A
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2190
How should this be tested?
covered by unit tests
Screenshots (if appropriate)
N/A
Questions:
N/A