[Improvement] Optimize the use of QuotaManager #359
Conversation
|
cc @xianjingfeng Could you help me review this patch? |
| this.currentUserAndApp = quotaManager.getCurrentUserAndApp(); | ||
| this.appIdToUser = quotaManager.getAppIdToUser(); | ||
| this.defaultUserApps = quotaManager.getDefaultUserApps(); | ||
| if (conf.get(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS).stream().map(String::trim).collect(Collectors.toList()) |
There was a problem hiding this comment.
String quotaCheckerClass = AccessQuotaChecker.class.getCanonicalName();
for (String checker : conf.get(COORDINATOR_ACCESS_CHECKERS)) {
if (quotaCheckerClass.equals(checker.trim())) {
this.quotaManager = new QuotaManager(conf);
this.currentUserAndApp = quotaManager.getCurrentUserAndApp();
this.appIdToUser = quotaManager.getAppIdToUser();
this.defaultUserApps = quotaManager.getDefaultUserApps();
break;
}
}Do you think it's good to write like this?
There was a problem hiding this comment.
It's ok for me.
| this.quotaManager = new QuotaManager(conf); | ||
| this.currentUserAndApp = quotaManager.getCurrentUserAndApp(); | ||
| this.appIdToUser = quotaManager.getAppIdToUser(); | ||
| this.defaultUserApps = quotaManager.getDefaultUserApps(); |
There was a problem hiding this comment.
Can we remove defaultUserApps in this class?
There was a problem hiding this comment.
Yes, this's not used in this class.
| @@ -53,13 +53,14 @@ public class QuotaManager { | |||
| private final Map<String, Integer> defaultUserApps = Maps.newConcurrentMap(); | |||
|
|
|||
| public QuotaManager(CoordinatorConf conf) { | |||
| this.quotaFilePath = conf.get(CoordinatorConf.COORDINATOR_QUOTA_DEFAULT_PATH);; | |||
| this.quotaFilePath = conf.get(CoordinatorConf.COORDINATOR_QUOTA_DEFAULT_PATH); | |||
There was a problem hiding this comment.
If quotaFilePath is empty, should we throw an exception here right now?
There was a problem hiding this comment.
If the quotaFilePath is empty, the quota for each user is the default value of rss.coordinator.quota.default.app.num. No exception is thrown, but logs can be added
| final Long updateTime = conf.get(CoordinatorConf.COORDINATOR_QUOTA_UPDATE_INTERVAL); | ||
| try { | ||
| hadoopFileSystem = HadoopFilesystemProvider.getFilesystem(new Path(quotaFilePath), new Configuration()); | ||
| } catch (Exception e) { | ||
| LOG.error("Cannot init remoteFS on path : " + quotaFilePath, e); | ||
| } | ||
| LOG.warn("We will update the quota of users regularly."); |
There was a problem hiding this comment.
This is to prompt the user that QuotaManager starts normally.
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
============================================
+ Coverage 58.32% 61.62% +3.29%
- Complexity 1550 1572 +22
============================================
Files 193 187 -6
Lines 10754 9685 -1069
Branches 936 954 +18
============================================
- Hits 6272 5968 -304
+ Misses 4109 3403 -706
+ Partials 373 314 -59
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Thanks all. @jerqi @xianjingfeng |
What changes were proposed in this pull request?
QuotaManager, useQuotaManagerwhen checking the number of apps, and decouple withApplicationManager.QuotaManageris controlled by the configuration of the checker.Why are the changes needed?
More reasonable.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added new ut.