You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/10/08 04:04:39 UTC
[GitHub] [druid] arunramani opened a new pull request #11783: Minor processor quota computation fix + docs
arunramani opened a new pull request #11783:
URL: https://github.com/apache/druid/pull/11783
### Description
Set processor quota to `-1` if quota is negative or period is 0. Added docs on how to enable the monitors
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] been tested in a test Druid cluster.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] techdocsmith commented on a change in pull request #11783: Minor processor quota computation fix + docs
Posted by GitBox <gi...@apache.org>.
techdocsmith commented on a change in pull request #11783:
URL: https://github.com/apache/druid/pull/11783#discussion_r725181445
##########
File path: docs/operations/metrics.md
##########
@@ -325,8 +325,8 @@ These metrics are only available if the SysMonitor module is included.
|`sys/swap/pageOut`|Paged out swap.||Varies.|
|`sys/disk/write/count`|Writes to disk.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
|`sys/disk/read/count`|Reads from disk.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
-|`sys/disk/write/size`|Bytes written to disk. Can we used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
-|`sys/disk/read/size`|Bytes read from disk. Can we used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
+|`sys/disk/write/size`|Bytes written to disk. Can be used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
Review comment:
```suggestion
|`sys/disk/write/size`|Bytes written to disk. One indicator of the amount of paging occurring for segments.`|fsDevName`,`fsDirName`,`fsTypeName`, `fsSysTypeName`, `fsOptions`.|Varies.|
```
##########
File path: docs/operations/metrics.md
##########
@@ -325,8 +325,8 @@ These metrics are only available if the SysMonitor module is included.
|`sys/swap/pageOut`|Paged out swap.||Varies.|
|`sys/disk/write/count`|Writes to disk.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
|`sys/disk/read/count`|Reads from disk.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
-|`sys/disk/write/size`|Bytes written to disk. Can we used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
-|`sys/disk/read/size`|Bytes read from disk. Can we used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
+|`sys/disk/write/size`|Bytes written to disk. Can be used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
+|`sys/disk/read/size`|Bytes read from disk. Can be used to determine how much paging is occurring with regards to segments.|fsDevName, fsDirName, fsTypeName, fsSysTypeName, fsOptions.|Varies.|
Review comment:
```suggestion
|`sys/disk/read/size`|Bytes read from disk. One indicator of the amount of paging occurring for segments.|`fsDevName`,`fsDirName`, `fsTypeName`, `fsSysTypeName`, `fsOptions`.|Varies.|
```
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis closed pull request #11783: Minor processor quota computation fix + docs
Posted by GitBox <gi...@apache.org>.
clintropolis closed pull request #11783:
URL: https://github.com/apache/druid/pull/11783
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei merged pull request #11783: Minor processor quota computation fix + docs
Posted by GitBox <gi...@apache.org>.
jon-wei merged pull request #11783:
URL: https://github.com/apache/druid/pull/11783
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11783: Minor processor quota computation fix + docs
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11783:
URL: https://github.com/apache/druid/pull/11783#discussion_r724707831
##########
File path: docs/operations/metrics.md
##########
@@ -336,3 +336,17 @@ These metrics are only available if the SysMonitor module is included.
|`sys/storage/used`|Disk space used.|fsDirName.|Varies.|
|`sys/cpu`|CPU used.|cpuName, cpuTime.|Varies.|
+## Cgroup
+
+These metrics are available on operating systems with the cgroup kernel feature. All the values are derived by reading from `/sys/fs/cgroup`.
+
+|Metric|Description|Dimensions|Normal Value|
+|------|-----------|----------|------------|
+|`cgroup/cpu/shares`|Relative value of CPU time available to this process. Read from `cpu.shares`.||Varies.|
+|`cgroup/cpu/cores_quota`|Number of cores available to this process. Derived from `cpu.cfs_quota_us`/`cpu.cfs_period_us`.||Varies.|
Review comment:
can you add a note about does `-1` value for this metric implies?
##########
File path: core/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java
##########
@@ -65,12 +65,23 @@ public boolean doMonitor(ServiceEmitter emitter)
emitter.emit(builder.build("cgroup/cpu/shares", cpuSnapshot.getShares()));
emitter.emit(builder.build(
"cgroup/cpu/cores_quota",
- cpuSnapshot.getPeriodUs() == 0
- ? 0
- : ((double) cpuSnapshot.getQuotaUs()
- ) / cpuSnapshot.getPeriodUs()
+ computeProcessorQuota(cpuSnapshot.getQuotaUs(), cpuSnapshot.getPeriodUs())
));
return true;
}
+
+ /**
+ * Calculates the total cores allocated through quotas.
+ *
+ * @param quotaUs the cgroup quota value.
+ * @param periodUs the cgroup period value.
+ * @return the calculated processor quota, -1 if no quota or period set.
+ */
+ public static double computeProcessorQuota(long quotaUs, long periodUs)
Review comment:
can you add some docs about why it should be `-1`?
##########
File path: core/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java
##########
@@ -65,12 +65,23 @@ public boolean doMonitor(ServiceEmitter emitter)
emitter.emit(builder.build("cgroup/cpu/shares", cpuSnapshot.getShares()));
emitter.emit(builder.build(
"cgroup/cpu/cores_quota",
- cpuSnapshot.getPeriodUs() == 0
- ? 0
- : ((double) cpuSnapshot.getQuotaUs()
- ) / cpuSnapshot.getPeriodUs()
+ computeProcessorQuota(cpuSnapshot.getQuotaUs(), cpuSnapshot.getPeriodUs())
));
return true;
}
+
+ /**
+ * Calculates the total cores allocated through quotas.
+ *
+ * @param quotaUs the cgroup quota value.
+ * @param periodUs the cgroup period value.
+ * @return the calculated processor quota, -1 if no quota or period set.
+ */
+ public static double computeProcessorQuota(long quotaUs, long periodUs)
+ {
+ return quotaUs < 0 || periodUs == 0
Review comment:
can we use two values here? So if `quotaUs` < 0, we return `-1` and if `periodUs` = 0, we return `-2`. This can be useful in troubleshooting. I suppose the absolute value of these returned value is not going to be used anywhere.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] arunramani commented on a change in pull request #11783: Minor processor quota computation fix + docs
Posted by GitBox <gi...@apache.org>.
arunramani commented on a change in pull request #11783:
URL: https://github.com/apache/druid/pull/11783#discussion_r725114303
##########
File path: core/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java
##########
@@ -65,12 +65,23 @@ public boolean doMonitor(ServiceEmitter emitter)
emitter.emit(builder.build("cgroup/cpu/shares", cpuSnapshot.getShares()));
emitter.emit(builder.build(
"cgroup/cpu/cores_quota",
- cpuSnapshot.getPeriodUs() == 0
- ? 0
- : ((double) cpuSnapshot.getQuotaUs()
- ) / cpuSnapshot.getPeriodUs()
+ computeProcessorQuota(cpuSnapshot.getQuotaUs(), cpuSnapshot.getPeriodUs())
));
return true;
}
+
+ /**
+ * Calculates the total cores allocated through quotas.
+ *
+ * @param quotaUs the cgroup quota value.
+ * @param periodUs the cgroup period value.
+ * @return the calculated processor quota, -1 if no quota or period set.
+ */
+ public static double computeProcessorQuota(long quotaUs, long periodUs)
+ {
+ return quotaUs < 0 || periodUs == 0
Review comment:
A single negative value should be enough. The idea is to indicate that there's no quota.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org