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-2713] Use queue specific REST API directly #878

Closed
wants to merge 3 commits into from

Conversation

blueBlue0102
Copy link
Contributor

What is this PR for?

There are some places in e2e tests using old way to fetching all queues for the given partition, then fetch queue specific info in next call. Instead, Queue info can be fetched directly in a single call.

What type of PR is it?

  • - Refactoring

What is the Jira issue?

YUNIKORN-2713

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.27%. Comparing base (f281908) to head (65e1520).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #878      +/-   ##
==========================================
- Coverage   67.30%   67.27%   -0.03%     
==========================================
  Files          70       70              
  Lines        7625     7625              
==========================================
- Hits         5132     5130       -2     
- Misses       2274     2276       +2     
  Partials      219      219              

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

Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@blueBlue0102 thanks for this patch

if queueName == "root" {
return queues, nil
}
// Include the query string 'subtree' to retrieve all queue children by default for better convenience in E2E tests
Copy link
Member

Choose a reason for hiding this comment

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

why we need this? The Restful API queue/${queue} already returns what you want, right?

Copy link
Contributor Author

@blueBlue0102 blueBlue0102 Jul 17, 2024

Choose a reason for hiding this comment

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

My intention was to make it easier for anyone who might use this utility method in the future. If a test case needs to check the queue's children and finds that this method doesn't retrieve them, it could be confusing, I think.

If this concern is unnecessary, I'm happy to remove this code. Or maybe GetQueue() could have an additional parameter: bool withChildren to remind callers and avoid potential confusion.

Copy link
Member

Choose a reason for hiding this comment

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

could have an additional parameter: bool withChildren to remind callers and avoid potential confusion.

that is good to me

Copy link

@baconYao baconYao left a comment

Choose a reason for hiding this comment

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

Hi @blueBlue0102, please see my suggestion.

test/e2e/framework/helpers/yunikorn/rest_api_utils.go Outdated Show resolved Hide resolved
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.

+1 LGTM

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.

4 participants