Skip to content

[#1178] improvement: set rss.coordinator.quota.default.app.num default -1 to indicate no quota check - #1186

Merged
roryqi merged 3 commits into
apache:masterfrom
xumanbu:1178_default_user_app_quta
Sep 4, 2023
Merged

[#1178] improvement: set rss.coordinator.quota.default.app.num default -1 to indicate no quota check#1186
roryqi merged 3 commits into
apache:masterfrom
xumanbu:1178_default_user_app_quta

Conversation

@xumanbu

@xumanbu xumanbu commented Sep 4, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

1、set rss.coordinator.quota.default.app.num default value as -1
2、the logic for quota check is the user app quota value < 0, then this user app quota check would passed

Why are the changes needed?

(Please clarify why the changes are needed. For instance,

  1. If you propose a new API, clarify the use case for a new API.
  2. If you fix a bug, describe the bug.)

Fix: # (#1178 )

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT

applicationManager.getQuotaManager().checkQuota("user4", String.valueOf(i1));
registerThread.join();
assertTrue(icCheck);
assertFalse(icCheck);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a case for rss.coordinator.quota.default.app.num = 5?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll keep current testCheckQuota logic for test rss.coordinator.quota.default.app.num = 5, add a new testCheckQuotaWithDefault function to test default value -1.

@codecov-commenter

codecov-commenter commented Sep 4, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1186 (31fe8f8) into master (ad9d0f1) will increase coverage by 1.08%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1186      +/-   ##
============================================
+ Coverage     53.63%   54.71%   +1.08%     
  Complexity     2581     2581              
============================================
  Files           391      371      -20     
  Lines         22407    20049    -2358     
  Branches       1875     1875              
============================================
- Hits          12017    10969    -1048     
+ Misses         9683     8442    -1241     
+ Partials        707      638      -69     
Files Changed Coverage Δ
...rg/apache/uniffle/coordinator/CoordinatorConf.java 98.78% <100.00%> (ø)
...a/org/apache/uniffle/coordinator/QuotaManager.java 83.54% <100.00%> (ø)

... and 23 files with indirect coverage changes

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

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @xumanbu

@roryqi
roryqi merged commit a3e2c91 into apache:master Sep 4, 2023
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.

3 participants