[GitHub] [pulsar] coderzc commented on a diff in pull request #16832: [feature][broker] Support cgroup v2 by using `jdk.internal.platform.Metrics` in Pulsar Loadbalancer

2023-04-28 Thread via GitHub 


coderzc commented on code in PR #16832:
URL: https://github.com/apache/pulsar/pull/16832#discussion_r1180389710


##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LinuxInfoUtils.java:
##
@@ -98,11 +138,14 @@ public static double getTotalCpuLimit(boolean 
isCGroupsEnabled) {
  * Get CGroup cpu usage.
  * @return Cpu usage
  */
-public static double getCpuUsageForCGroup() {
+public static long getCpuUsageForCGroup() {
 try {
+if (metrics != null && getCpuUsageMethod != null) {
+return (long) getCpuUsageMethod.invoke(metrics);

Review Comment:
   The `Metrics.getCpuUsage` will return the aggregate time, so I think we 
don't need to multiply the limit.
   
   ```
   /**
* Returns the aggregate time, in nanoseconds, consumed by all
* tasks in the Isolation Group.
*
* @return Time in nanoseconds, -1 if unknown or
* -2 if the metric is not supported.
*
*/
   public long getCpuUsage();
   ```



##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LinuxInfoUtils.java:
##
@@ -98,11 +138,14 @@ public static double getTotalCpuLimit(boolean 
isCGroupsEnabled) {
  * Get CGroup cpu usage.
  * @return Cpu usage
  */
-public static double getCpuUsageForCGroup() {
+public static long getCpuUsageForCGroup() {
 try {
+if (metrics != null && getCpuUsageMethod != null) {
+return (long) getCpuUsageMethod.invoke(metrics);

Review Comment:
   The `Metrics.getCpuUsage` will return the aggregate time, so I think we 
don't need to multiply the limit.
   
   ```java
   /**
* Returns the aggregate time, in nanoseconds, consumed by all
* tasks in the Isolation Group.
*
* @return Time in nanoseconds, -1 if unknown or
* -2 if the metric is not supported.
*
*/
   public long getCpuUsage();
   ```



-- 
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: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #16832: [feature][broker] Support cgroup v2 by using `jdk.internal.platform.Metrics` in Pulsar Loadbalancer

2022-08-14 Thread GitBox 


coderzc commented on code in PR #16832:
URL: https://github.com/apache/pulsar/pull/16832#discussion_r945395452


##
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/SimpleBrokerStartTest.java:
##
@@ -97,4 +101,31 @@ public void testNoNICSpeed() throws Exception {
 }
 
 
+@Test
+public void testCGroupMetrics() throws IllegalAccessException {
+if (!LinuxInfoUtils.isLinux()) {
+return;
+}
+
+boolean existsCGroup = Files.exists(Paths.get("/sys/fs/cgroup"));
+boolean cGroupEnabled = LinuxInfoUtils.isCGroupEnabled();
+Assert.assertEquals(cGroupEnabled, existsCGroup);
+
+double totalCpuLimit = LinuxInfoUtils.getTotalCpuLimit(cGroupEnabled);
+log.info("totalCpuLimit: {}", totalCpuLimit);
+Assert.assertTrue(totalCpuLimit > 0.0);
+
+if (cGroupEnabled) {
+
Assert.assertNotNull(FieldUtils.readStaticField(LinuxInfoUtils.class, 
"metrics", true));

Review Comment:
   Ok, it makes sense.



-- 
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: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #16832: [feature][broker] Support cgroup v2 by using `jdk.internal.platform.Metrics` in Pulsar Loadbalancer

2022-08-10 Thread GitBox 


coderzc commented on code in PR #16832:
URL: https://github.com/apache/pulsar/pull/16832#discussion_r943076687


##
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/SimpleBrokerStartTest.java:
##
@@ -97,4 +101,31 @@ public void testNoNICSpeed() throws Exception {
 }
 
 
+@Test
+public void testCGroupMetrics() throws IllegalAccessException {
+if (!LinuxInfoUtils.isLinux()) {
+return;
+}
+
+boolean existsCGroup = Files.exists(Paths.get("/sys/fs/cgroup"));
+boolean cGroupEnabled = LinuxInfoUtils.isCGroupEnabled();
+Assert.assertEquals(cGroupEnabled, existsCGroup);
+
+double totalCpuLimit = LinuxInfoUtils.getTotalCpuLimit(cGroupEnabled);
+log.info("totalCpuLimit: {}", totalCpuLimit);
+Assert.assertTrue(totalCpuLimit > 0.0);
+
+if (cGroupEnabled) {
+
Assert.assertNotNull(FieldUtils.readStaticField(LinuxInfoUtils.class, 
"metrics", true));

Review Comment:
   Why not use reflection to get it? This is just a test.



-- 
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: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #16832: [feature][broker] Support cgroup v2 by using `jdk.internal.platform.Metrics` in Pulsar Loadbalancer

2022-08-08 Thread GitBox 


coderzc commented on code in PR #16832:
URL: https://github.com/apache/pulsar/pull/16832#discussion_r940183684


##
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/SimpleBrokerStartTest.java:
##
@@ -97,4 +101,25 @@ public void testNoNICSpeed() throws Exception {
 }
 
 
+@Test
+public void testCGroupMetrics() throws IOException {

Review Comment:
   Good idea, we can assert `metrics != 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: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #16832: [feature][broker] Support cgroup v2 by using `jdk.internal.platform.Metrics` in Pulsar Loadbalancer

2022-08-02 Thread GitBox 


coderzc commented on code in PR #16832:
URL: https://github.com/apache/pulsar/pull/16832#discussion_r935777436


##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LinuxInfoUtils.java:
##
@@ -65,9 +87,14 @@ public static boolean isLinux() {
  */
 public static boolean isCGroupEnabled() {
 try {
-return Files.exists(Paths.get(CGROUPS_CPU_USAGE_PATH));
+if (metrics == null) {
+return false;

Review Comment:
   Thanks, I updated 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: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org