You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "smallzhongfeng (via GitHub)" <gi...@apache.org> on 2023/02/28 17:40:28 UTC
[GitHub] [incubator-uniffle] smallzhongfeng opened a new pull request, #672: [#671] Metrics of the number of apps submitted by users
smallzhongfeng opened a new pull request, #672:
URL: https://github.com/apache/incubator-uniffle/pull/672
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
Examples:
- "[#123] feat(operator): support xxx"
- "[#233] fix: check null before access result in xxx"
- "[MINOR] refactor: fix typo in variable name"
- "[MINOR] docs: fix typo in README"
- "[#255] test: fix flaky test NameOfTheTest"
Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. Contributor guidelines:
https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md
3. If the PR is unfinished, please mark this PR as draft.
-->
### What changes were proposed in this pull request?
Add metrics for `QuotaManager` to record the number of apps run by each user.
### Why are the changes needed?
Fix: #671
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New uts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121233422
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
Maybe not. I must set this value after the add metric, but the `computeIfAbsent` method must return type `Guage`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121094508
##########
common/src/main/java/org/apache/uniffle/common/security/HadoopSecurityContext.java:
##########
@@ -56,7 +56,7 @@ public HadoopSecurityContext(
if (StringUtils.isNotEmpty(krb5ConfPath)) {
System.setProperty(KRB5_CONF_KEY, krb5ConfPath);
- sun.security.krb5.Config.refresh();
+// sun.security.krb5.Config.refresh();
Review Comment:
Why do we change this line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121021544
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
+ }
+ }
+
+ public static void updateDynamicGaugeForUser(String user, double value) {
+ if (GAUGE_APP_NUM_TO_USER.containsKey(user)) {
Review Comment:
```suggestion
Gauge metric = GAUGE_APP_NUM_TO_USER.get(user)
if (metric != null ) {
.....
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121019681
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
Could we merge this line to the pre line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1123344746
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
No. You can create a Gauge with `user` or `user_name` label once.
Then you can update the gauge with user_name label set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122552968
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java:
##########
@@ -140,6 +143,24 @@ public void registerApplicationInfo(String appId, Map<String, Long> appAndTime)
}
}
+ protected void updateQuotaMetrics() {
+ for (Map.Entry<String, Map<String, Long>> userAndApp : currentUserAndApp.entrySet()) {
+ String user = userAndApp.getKey();
+ try {
+ CoordinatorMetrics.updateDynamicGaugeForUser(user, userAndApp.getValue().size());
+ } catch (Exception e) {
+ LOG.warn("Update user metrics for {} failed ", user, e);
+ }
+ }
+ }
+
+ protected void addQuotaMetrics(String user) {
+ if (StringUtils.isNotEmpty(user)) {
Review Comment:
Before the client communicates with coordinator, the user will be set to conf. If the user is null, the client will throw a nullpointer.
```sparkConf.set("spark.rss.quota.user", user);```
So this place should not be empty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng merged pull request #672: [#671] feat(coordinator): Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng merged PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121926956
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
how about add label for this gauge? such as app_num{user_id=xxx}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121815887
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
Review Comment:
I don't think it is necessary here, because it needs to introduce `MetricsManager` into `QuotaManager`, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121806813
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (StringUtils.isNotEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
+ }
+ }
+
+ public static void updateDynamicGaugeForUser(String user, double value) {
+ Gauge gauge = GAUGE_APP_NUM_TO_USER.get(user);
+ if (gauge != null) {
+ GAUGE_APP_NUM_TO_USER.get(user).set(value);
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122122267
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
Could you give me an example, I haven't used it before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121073696
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
+ }
+ }
+
+ public static void updateDynamicGaugeForUser(String user, double value) {
+ if (GAUGE_APP_NUM_TO_USER.containsKey(user)) {
Review Comment:
I got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121177419
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121098769
##########
common/src/main/java/org/apache/uniffle/common/security/HadoopSecurityContext.java:
##########
@@ -56,7 +56,7 @@ public HadoopSecurityContext(
if (StringUtils.isNotEmpty(krb5ConfPath)) {
System.setProperty(KRB5_CONF_KEY, krb5ConfPath);
- sun.security.krb5.Config.refresh();
+// sun.security.krb5.Config.refresh();
Review Comment:
Sorry, I just commented it to run out, and it has been rolled back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121759107
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122519399
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java:
##########
@@ -140,6 +143,24 @@ public void registerApplicationInfo(String appId, Map<String, Long> appAndTime)
}
}
+ protected void updateQuotaMetrics() {
+ for (Map.Entry<String, Map<String, Long>> userAndApp : currentUserAndApp.entrySet()) {
+ String user = userAndApp.getKey();
+ try {
+ CoordinatorMetrics.updateDynamicGaugeForUser(user, userAndApp.getValue().size());
+ } catch (Exception e) {
+ LOG.warn("Update user metrics for {} failed ", user, e);
+ }
+ }
+ }
+
+ protected void addQuotaMetrics(String user) {
+ if (StringUtils.isNotEmpty(user)) {
Review Comment:
Em....we would better not. Because MR don't support quota now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1124059484
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
It's a good way. Thanks !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121085901
##########
coordinator/src/test/java/org/apache/uniffle/coordinator/QuotaManagerTest.java:
##########
@@ -98,4 +111,37 @@ public void testCheckQuota() throws Exception {
assertTrue(icCheck);
assertEquals(applicationManager.getQuotaManager().getCurrentUserAndApp().get("user4").size(), 5);
}
+
+ @Test
+ public void testCheckQuotaMetrics() throws Exception {
+ CoordinatorConf conf = new CoordinatorConf();
+ conf.set(CoordinatorConf.COORDINATOR_QUOTA_DEFAULT_PATH, quotaFile);
+ conf.setLong(CoordinatorConf.COORDINATOR_APP_EXPIRED, 1500);
+ conf.setInteger(CoordinatorConf.COORDINATOR_QUOTA_DEFAULT_APP_NUM, 2);
+ final ApplicationManager applicationManager = new ApplicationManager(conf);
+ final AtomicInteger uuid = new AtomicInteger();
+ final int i1 = uuid.incrementAndGet();
+ final int i2 = uuid.incrementAndGet();
+ final int i3 = uuid.incrementAndGet();
+ Map<String, Long> uuidAndTime = new ConcurrentHashMap<>();
+ uuidAndTime.put(String.valueOf(i1), System.currentTimeMillis());
+ uuidAndTime.put(String.valueOf(i2), System.currentTimeMillis());
+ uuidAndTime.put(String.valueOf(i3), System.currentTimeMillis());
+ final boolean icCheck = applicationManager.getQuotaManager()
+ .checkQuota("user4", String.valueOf(i1));
+ final boolean icCheck2 = applicationManager.getQuotaManager()
+ .checkQuota("user4", String.valueOf(i2));
+ final boolean icCheck3 = applicationManager.getQuotaManager()
+ .checkQuota("user4", String.valueOf(i3));
+ assertFalse(icCheck);
+ assertFalse(icCheck2);
+ // The default number of tasks submitted is 2, and the third will be rejected
+ assertTrue(icCheck3);
+ assertEquals(applicationManager.getQuotaManager().getCurrentUserAndApp().get("user4").size(), 2);
+ assertEquals(CoordinatorMetrics.GAUGE_APP_NUM_TO_USER.get("user4").get(), 2);
+ Thread.sleep(2000);
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121610424
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
I mean
``` java
GAUGE_APP_NUM_TO_USER.computeIfAbsent(
user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122539443
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java:
##########
@@ -140,6 +143,24 @@ public void registerApplicationInfo(String appId, Map<String, Long> appAndTime)
}
}
+ protected void updateQuotaMetrics() {
+ for (Map.Entry<String, Map<String, Long>> userAndApp : currentUserAndApp.entrySet()) {
+ String user = userAndApp.getKey();
+ try {
+ CoordinatorMetrics.updateDynamicGaugeForUser(user, userAndApp.getValue().size());
+ } catch (Exception e) {
+ LOG.warn("Update user metrics for {} failed ", user, e);
+ }
+ }
+ }
+
+ protected void addQuotaMetrics(String user) {
+ if (StringUtils.isNotEmpty(user)) {
Review Comment:
But we also need to know how many applications are submitted by null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122666475
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
Please see org.apache.uniffle.common.metrics.MetricsManager's related `addGauge` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121024501
##########
coordinator/src/test/java/org/apache/uniffle/coordinator/QuotaManagerTest.java:
##########
@@ -98,4 +111,37 @@ public void testCheckQuota() throws Exception {
assertTrue(icCheck);
assertEquals(applicationManager.getQuotaManager().getCurrentUserAndApp().get("user4").size(), 5);
}
+
+ @Test
+ public void testCheckQuotaMetrics() throws Exception {
+ CoordinatorConf conf = new CoordinatorConf();
+ conf.set(CoordinatorConf.COORDINATOR_QUOTA_DEFAULT_PATH, quotaFile);
+ conf.setLong(CoordinatorConf.COORDINATOR_APP_EXPIRED, 1500);
+ conf.setInteger(CoordinatorConf.COORDINATOR_QUOTA_DEFAULT_APP_NUM, 2);
+ final ApplicationManager applicationManager = new ApplicationManager(conf);
+ final AtomicInteger uuid = new AtomicInteger();
+ final int i1 = uuid.incrementAndGet();
+ final int i2 = uuid.incrementAndGet();
+ final int i3 = uuid.incrementAndGet();
+ Map<String, Long> uuidAndTime = new ConcurrentHashMap<>();
+ uuidAndTime.put(String.valueOf(i1), System.currentTimeMillis());
+ uuidAndTime.put(String.valueOf(i2), System.currentTimeMillis());
+ uuidAndTime.put(String.valueOf(i3), System.currentTimeMillis());
+ final boolean icCheck = applicationManager.getQuotaManager()
+ .checkQuota("user4", String.valueOf(i1));
+ final boolean icCheck2 = applicationManager.getQuotaManager()
+ .checkQuota("user4", String.valueOf(i2));
+ final boolean icCheck3 = applicationManager.getQuotaManager()
+ .checkQuota("user4", String.valueOf(i3));
+ assertFalse(icCheck);
+ assertFalse(icCheck2);
+ // The default number of tasks submitted is 2, and the third will be rejected
+ assertTrue(icCheck3);
+ assertEquals(applicationManager.getQuotaManager().getCurrentUserAndApp().get("user4").size(), 2);
+ assertEquals(CoordinatorMetrics.GAUGE_APP_NUM_TO_USER.get("user4").get(), 2);
+ Thread.sleep(2000);
Review Comment:
Could we use await().atMost...?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121067413
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
Do you mean to merge 105 lines into 104 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121067579
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
Review Comment:
OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121777047
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (StringUtils.isNotEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
+ }
+ }
+
+ public static void updateDynamicGaugeForUser(String user, double value) {
+ Gauge gauge = GAUGE_APP_NUM_TO_USER.get(user);
+ if (gauge != null) {
+ GAUGE_APP_NUM_TO_USER.get(user).set(value);
Review Comment:
`gauge.set(value);`？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122540675
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
Review Comment:
OK. I misread it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#issuecomment-1453027552
PTAL @advancedxy @xianjingfeng @jerqi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121020233
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
Review Comment:
`isNotEmpty` may be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121772168
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java:
##########
@@ -140,6 +143,24 @@ public void registerApplicationInfo(String appId, Map<String, Long> appAndTime)
}
}
+ protected void updateQuotaMetrics() {
+ for (Map.Entry<String, Map<String, Long>> userAndApp : currentUserAndApp.entrySet()) {
+ String user = userAndApp.getKey();
+ try {
+ CoordinatorMetrics.updateDynamicGaugeForUser(user, userAndApp.getValue().size());
+ } catch (Exception e) {
+ LOG.warn("Update user metrics for {} failed ", user, e);
+ }
+ }
+ }
+
+ protected void addQuotaMetrics(String user) {
+ if (StringUtils.isNotEmpty(user)) {
Review Comment:
I think we should remove this condition. It is unreasonable for user to be empty. We should intercept it upstream. WDYT? @jerqi @smallzhongfeng
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121775783
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
Review Comment:
Should we merge this method to `addQuotaMetrics`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121610424
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,23 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ if (!StringUtils.isEmpty(user)) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user));
+ GAUGE_APP_NUM_TO_USER.get(user).inc();
Review Comment:
I mean GAUGE_APP_NUM_TO_USER.computeIfAbsent(
user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1123136528
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
Do you mean like this ?
```
public static void addDynamicGaugeForUser(String user) {
GAUGE_APP_NUM_TO_USER.computeIfAbsent(
user, x -> metricsManager.addGauge(APP_NUM_TO_USER, user)).labels("user_name").inc();
}
```
@advancedxy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1121803323
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java:
##########
@@ -140,6 +143,24 @@ public void registerApplicationInfo(String appId, Map<String, Long> appAndTime)
}
}
+ protected void updateQuotaMetrics() {
+ for (Map.Entry<String, Map<String, Long>> userAndApp : currentUserAndApp.entrySet()) {
+ String user = userAndApp.getKey();
+ try {
+ CoordinatorMetrics.updateDynamicGaugeForUser(user, userAndApp.getValue().size());
+ } catch (Exception e) {
+ LOG.warn("Update user metrics for {} failed ", user, e);
+ }
+ }
+ }
+
+ protected void addQuotaMetrics(String user) {
+ if (StringUtils.isNotEmpty(user)) {
Review Comment:
It is OK for me here. The user transmitted from the client has been null judged. So `null` should not appear here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#issuecomment-1448608410
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#672](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (475e97e) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/b3a10d39ce802e0bcf5bde822fdea95c9b3afcd2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b3a10d3) will **increase** coverage by `2.44%`.
> The diff coverage is `84.61%`.
```diff
@@ Coverage Diff @@
## master #672 +/- ##
============================================
+ Coverage 60.63% 63.08% +2.44%
- Complexity 1799 1816 +17
============================================
Files 216 202 -14
Lines 12455 10551 -1904
Branches 1052 1057 +5
============================================
- Hits 7552 6656 -896
+ Misses 4496 3546 -950
+ Partials 407 349 -58
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/uniffle/coordinator/QuotaManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvUXVvdGFNYW5hZ2VyLmphdmE=) | `83.95% <76.92%> (-1.35%)` | :arrow_down: |
| [...uniffle/coordinator/metric/CoordinatorMetrics.java](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvbWV0cmljL0Nvb3JkaW5hdG9yTWV0cmljcy5qYXZh) | `93.33% <90.90%> (-0.79%)` | :arrow_down: |
| [...apache/uniffle/coordinator/ApplicationManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQXBwbGljYXRpb25NYW5hZ2VyLmphdmE=) | `84.57% <100.00%> (+0.17%)` | :arrow_up: |
| [...pache/spark/shuffle/writer/WriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JpdGVCdWZmZXJNYW5hZ2VyLmphdmE=) | `83.09% <0.00%> (-1.19%)` | :arrow_down: |
| [...che/uniffle/coordinator/util/CoordinatorUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvdXRpbC9Db29yZGluYXRvclV0aWxzLmphdmE=) | `72.00% <0.00%> (ø)` | |
| [...pkg/controller/sync/shuffleserver/shuffleserver.go](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL2NvbnRyb2xsZXIvc3luYy9zaHVmZmxlc2VydmVyL3NodWZmbGVzZXJ2ZXIuZ28=) | | |
| [deploy/kubernetes/operator/pkg/utils/config.go](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL3V0aWxzL2NvbmZpZy5nbw==) | | |
| [deploy/kubernetes/operator/pkg/utils/certs.go](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL3V0aWxzL2NlcnRzLmdv) | | |
| [deploy/kubernetes/operator/pkg/webhook/manager.go](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL3dlYmhvb2svbWFuYWdlci5nbw==) | | |
| [...bernetes/operator/pkg/controller/controller/rss.go](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL2NvbnRyb2xsZXIvY29udHJvbGxlci9yc3MuZ28=) | | |
| ... and [15 more](https://codecov.io/gh/apache/incubator-uniffle/pull/672?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1122540334
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/QuotaManager.java:
##########
@@ -140,6 +143,24 @@ public void registerApplicationInfo(String appId, Map<String, Long> appAndTime)
}
}
+ protected void updateQuotaMetrics() {
+ for (Map.Entry<String, Map<String, Long>> userAndApp : currentUserAndApp.entrySet()) {
+ String user = userAndApp.getKey();
+ try {
+ CoordinatorMetrics.updateDynamicGaugeForUser(user, userAndApp.getValue().size());
+ } catch (Exception e) {
+ LOG.warn("Update user metrics for {} failed ", user, e);
+ }
+ }
+ }
+
+ protected void addQuotaMetrics(String user) {
+ if (StringUtils.isNotEmpty(user)) {
Review Comment:
OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1124059484
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
It's a good way. Thx !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on pull request #672: [#671] feat(coordinator): Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#issuecomment-1453784457
Thanks for your review! :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] smallzhongfeng commented on a diff in pull request #672: [#671] Metrics of the number of apps submitted by users
Posted by "smallzhongfeng (via GitHub)" <gi...@apache.org>.
smallzhongfeng commented on code in PR #672:
URL: https://github.com/apache/incubator-uniffle/pull/672#discussion_r1123140447
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/metric/CoordinatorMetrics.java:
##########
@@ -96,6 +98,20 @@ public static void updateDynamicGaugeForRemoteStorage(String storageHost, double
}
}
+ public static void addDynamicGaugeForUser(String user) {
+ GAUGE_APP_NUM_TO_USER.computeIfAbsent(
+ user, x -> metricsManager.addGauge(APP_NUM_TO_USER + user)).inc();
Review Comment:
Gauge name can only set one time, if we call `addDynamicGaugeForUser` for more time, it will throw `Collector already registered that provides name`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org