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.

Sign up for GitHub

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

Jump to bottom

[YUNIKORN-1923] update apps columns #131

Closed
wants to merge 1 commit into from
Closed

[YUNIKORN-1923] update apps columns #131

wants to merge 1 commit into from

Conversation

FrankYang0529
Copy link
Member

What is this PR for?

  • remove partition and queue columns
  • add pending resource column

What type of PR is it?

  • - Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1923

Screenshots (if appropriate)

Screenshot 2023-08-22 at 4 25 31 PM

craigcondit
craigcondit requested changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

Can we get some test data that exercises this in the json db ?

@FrankYang0529 FrankYang0529 force-pushed the YUNIKORN-1923 branch 5 times, most recently from 65dad1d to 6a5f366 Compare August 31, 2023 09:24
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2023 •
edited
Loading

Codecov Report

Merging #131 (f13eb50) into master (f510bd2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #131   +/-   ##
=======================================
  Coverage   66.66%   66.66%           
=======================================
  Files           1        1           
  Lines          30       30           
=======================================
  Hits           20       20           
  Misses          7        7           
  Partials        3        3

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@FrankYang0529
Copy link
Member Author

Hi @craigcondit, I added a test case for it. However, I'm not familiar with Angular. Not sure whether I did it right. Could you help me review again? Thank you.

@craigcondit
Copy link
Contributor

@FrankYang0529 not just the test case, but the example JSON data that is used to drive json-db. This allows us to visually see the impact of the changes in the UI.

@craigcondit
Copy link
Contributor

@FrankYang0529 can you fix the merge conflicts and update json-db with non-zero pending resources?

@FrankYang0529
Copy link
Member Author

@FrankYang0529 can you fix the merge conflicts and update json-db with non-zero pending resources?

Hi @craigcondit, thanks for the review. Also, sorry for late. I will resolve conflicts and update json-db tomorrow 👍 .

@FrankYang0529
Copy link
Member Author

@FrankYang0529 can you fix the merge conflicts and update json-db with non-zero pending resources?

Hi @craigcondit, thanks for the review. Also, sorry for late. I will resolve conflicts and update json-db tomorrow 👍 .

Hi @craigcondit, I resolved conflicts and add non-zero pending resources to json-db. Could you review it again when you have time? Thank you.

0yukali0
0yukali0 requested changes Oct 1, 2023
View reviewed changes
Copy link
Contributor

@0yukali0 0yukali0 left a comment

Choose a reason for hiding this comment

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

I ran the test and the make start-dev locally to check the result.
The result meets the description in this pull request.

json-db.json Outdated
@@ -512,6 +512,46 @@
"hasReserved": false,
"reservations": [],
"maxRequestPriority": -2147483648
},
{
"applicationID": "application-sleep-0002",
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already an app-0002 in the database.
For distinguishing the applications, replace 0002 with 0006.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it. Thank you.

@FrankYang0529
[YUNIKORN-1923] update apps columns
f13eb50 
* remove partition and queue columns
* add pending resource column

Signed-off-by: PoAn Yang <payang@apache.org>
@wilfred-s
Copy link
Contributor

This looks like it is all done. Can we get a final review of the changes as I think all requested changes are made

wilfred-s
wilfred-s approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

LGTM, +1

@wilfred-s wilfred-s closed this in 32ad02b Oct 17, 2023
@FrankYang0529 FrankYang0529 deleted the YUNIKORN-1923 branch October 17, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilfred-s wilfred-s wilfred-s approved these changes

@lixmgl lixmgl Awaiting requested review from lixmgl

@craigcondit craigcondit Awaiting requested review from craigcondit

@0yukali0 0yukali0 Awaiting requested review from 0yukali0

Assignees

@FrankYang0529 FrankYang0529

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@FrankYang0529 @codecov-commenter @craigcondit @wilfred-s @0yukali0