Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YUNIKORN-2461] Add new RESTful API to get queue applications filtered by state #861

Closed
wants to merge 2 commits into from

Conversation

targetoee
Copy link
Contributor

What is this PR for?

In YUNIKORN-2235, an API to retrieve applications by state in partition was added. However, there is currently no API serving the same purpose at the queue level.
This PR add a new RESTful API that allows filtering applications by state at queue level, which unifies API behavior.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

N/A

What is the Jira issue?

YUNIKORN-2461

How should this be tested?

It has been checked locally by make test.

Screenshots (if appropriate)

N/A

Questions:

N/A

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 76.97%. Comparing base (cf838f9) to head (507da8b).
Report is 6 commits behind head on master.

Files Patch % Lines
pkg/webservice/handlers.go 66.66% 9 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
- Coverage   77.00%   76.97%   -0.03%     
==========================================
  Files          97       97              
  Lines       12002    12040      +38     
==========================================
+ Hits         9242     9268      +26     
- Misses       2422     2430       +8     
- Partials      338      342       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenyulin0719
Copy link
Contributor

chenyulin0719 commented May 16, 2024

I have another thought.
Since we only have 'active' applications in queue, how about extend the existing 'queue applications' api?

Extend

to

Keep using query parameter to filter out 'new', 'accepted', 'running', 'completing', 'failing', 'resuming'.

@pbacsko
Copy link
Contributor

pbacsko commented Jun 20, 2024

@targetoee could you rebase this patch to master?

@pbacsko pbacsko self-requested a review June 20, 2024 15:05
Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.


switch status {
case "Rejected":
buildJSONErrorResponse(w, "Queue does not involve rejected state applications.", http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better approach:

switch status {
  case "rejected", "completed":
    buildJSONErrorResponse(w, fmt.Sprintf("Unsupported application state in queue: %s", status), http.StatusBadRequest)
   return
  default:
  ...
}

}
partition := vars.ByName("partition")
queueName := vars.ByName("queue")
status := vars.ByName("status")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be converted to full lowercase with strings.ToLower().

@pbacsko
Copy link
Contributor

pbacsko commented Jun 25, 2024

@targetoee could you rebase this PR?

@blueBlue0102
Copy link
Contributor

After contacting @targetoee and obtaining his consent, I will proceed to complete this issue.

I also agree with @chenyulin0719's suggestion.
I believe the API path /partition/{partitionName}/queue/{queueName}/applications/{state}?status={status} might be a more suitable approach.
This structure maintains better consistency with the non-queue version: /partition/{partitionName}/applications/{state}?status={status}

I'd appreciate hearing everyone's thoughts on this proposal. @pbacsko, WDYT?

@chenyulin0719
Copy link
Contributor

@blueBlue0102 Thanks for taking over this Jira.
As we discussed,
In YUNIKORN-2235, we have below new APIs.

This Jira (YUNIKORN-2461) should be implemented in the consistent way:

(My previous comment could be ignored)

Please proceed and update the original description in this Jira. Thanks.

@chia7712
Copy link
Contributor

close this PR as duplicate to #938

@chia7712 chia7712 closed this Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants