Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2069517654 @emitskevich-blp Thanks for fixing the incorrect docs :) -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 merged PR #15722: URL: https://github.com/apache/kafka/pull/15722 -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2069514738 all failed tests pass on my local. merge 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573473787
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
That's the elegant solution I was looking for :)
Applied.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573465932
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
For example:
```java
assertEquals(actual.size(), metrics.metrics().keySet().stream()
.filter(m -> actual.contains(m.name()))
.filter(m ->
!m.description().toLowerCase(Locale.ROOT).contains("deprecated"))
.count());
assertEquals(deprecated.size(), metrics.metrics().keySet().stream()
.filter(m -> deprecated.contains(m.name()))
.filter(m ->
m.description().toLowerCase(Locale.ROOT).contains("deprecated"))
.count());
```
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573465553
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
> However, I'm concerning that if at some point all of them are removed, the
test will stop doing anything meaningful without showing any sign of it. But I
don't see an elegant trigger to show it.
It seems to me making the test failed can remind developer - "oh, this test
is failed and it unnecessary since I have removed those metrics from
production".
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573458599
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
I'm not sure we should. Could you please elaborate more? Below are my
thoughts.
What is expected to be verified by new suggested check?
1. Absence of human mistake during writing the test? Then it may become
similar to testing the tests.
2. Actual presence of all these metrics in the registry? But I think this
particular test shouldn't care if some of them are removed. The goal here is
"for these names, if they are registered, we expect such docs".
However, I'm concerning that if at some point **all of them** are removed,
the test will stop doing anything meaningful without showing any sign of it.
But I don't see an elegant trigger to show 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573458599
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
I'm not sure we should. Could you please elaborate more? Below are my
thoughts.
What is expected to be verified by new suggested check?
1. Absence of human mistake during writing the test? Then it may become
similar to testing the tests.
2. Actual presence of all these metrics in the registry? But I think this
particular test shouldn't care if some of them are removed. The goal here is
"for these names, if they are registered, we expect such docs".
However, I'm concerning that if at some point **all of them** are removed,
the test will stop doing anything meaningful without showing any sign of 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573458599
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
I'm not sure we should. Could you please elaborate more? Below are my
thoughts.
What is expected to be verified by new suggested check?
1. Absence of human mistake during writing the test? Then it may become
similar to testing the tests.
2. Actual presence of all these metrics in the registry? But I think this
particular test shouldn't care if some of them are removed at some point. The
goal here is "for these names, if they are registered, we expect such docs".
However, I'm concerning that if at some point **all of them** are removed,
the test will stop doing anything meaningful without showing any sign of 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573454740
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Ok, moved to inline comments
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1573445048
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Can we move the comments to here instead of modifying the method docs?
##
clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java:
##
@@ -1012,6 +1013,28 @@ public void testChannelCloseWhileProcessingReceives()
throws Exception {
assertEquals(0, selector.completedReceives().size());
}
+/**
+ * Validate that correct subset of io metrics marked deprecated in docs
+ */
+@Test
+public void testIoMetricsHaveCorrectDoc() {
+List actual = asList("io-ratio", "io-wait-ratio");
+List deprecated = asList("iotime-total", "io-waittime-total");
+
+// iterate through all metrics, since metrics.metric(metricName)
requires
+// knowing metric group and tag set, which are not trivial dependencies
+for (MetricName metricName : metrics.metrics().keySet()) {
Review Comment:
Could we make sure both `actual` and `deprecated` get verified? For example,
this pass can pass if we use incorrect metrics name. i.e `if
(actual.contains(name))` and `if (deprecated.contains(name))` return false.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2067299812 > we should verify the non-deprecated metrics should have correct doc which is not marked as "deprecated". Also, that is what you try to fix, right? Correct, this is the goal, yes. I got wrong what you mean by "doc". Added test to validate metric descriptions. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2066283854 > Effectively, such test would verify the behavior of deprecated method. What do you think? we should verify the non-deprecated metrics should have correct doc which is not marked as "deprecated". Also, that is what you try to fix, 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2064749720 Is it possible to add unit test? > I'm not sure whether it's needed here. Effectively, such test would verify the behavior of deprecated method. What do you think? We should verify that docs should not include deprecated > The docs are correct: [io-wait-ratio](https://github.com/emitskevich-blp/kafka/blob/53ff1a5a589eb0e30a302724fcf5d0c72c823682/docs/ops.html#L2372), [io-ratio](https://github.com/emitskevich-blp/kafka/blob/53ff1a5a589eb0e30a302724fcf5d0c72c823682/docs/ops.html#L2392) -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1571063662
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Added to method javadoc
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1571063662
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Added in method javadoc
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1569005969
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Please find it in PR description
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1569005969
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Hi, please find it in PR description
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
Please find it in PR description
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
chia7712 commented on code in PR #15722:
URL: https://github.com/apache/kafka/pull/15722#discussion_r1568836486
##
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##
@@ -1281,14 +1281,14 @@ private Meter createMeter(Metrics metrics, String
groupName, Map metricTags,
String baseName, String action) {
MetricName rateMetricName = metrics.metricName(baseName +
"-ratio", groupName,
-String.format("*Deprecated* The fraction of time the I/O
thread spent %s", action), metricTags);
+String.format("The fraction of time the I/O thread spent
%s", action), metricTags);
Review Comment:
nice finding! Could you add comments to explain why we remove the
`deprecated`?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2059230580 This is potentially related to [KIP-773 changes](https://github.com/apache/kafka/pull/11302) @jlprat -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp commented on PR #15722: URL: https://github.com/apache/kafka/pull/15722#issuecomment-2059223960 Tests failure is unrelated to PR changes -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] MINOR: Fix io-[wait-]ratio metrics description [kafka]
emitskevich-blp opened a new pull request, #15722: URL: https://github.com/apache/kafka/pull/15722 [Implementation of KIP-773](https://github.com/apache/kafka/pull/11302) deprecated `iotime-total` and `io-waittime-total` metrics. It wasn't expected to mark `io-ratio` and `io-wait-ratio` deprecated. However, now they have `*Deprecated* ` in their description. Here is the reason: 1. register `io-ratio` (desc: `*Deprecated* The fraction of time ...`) -> registered 2. register `iotime-total` (desc: `*Deprecated* The total time ...`) -> registered 3. register `io-ratio` (desc: `The fraction of time ...`) -> **skipped, the same name already exists in registry** 4. register `io-time-ns-total` (desc: `The total time ...`) -> registered As a result, `io-ratio` has incorrect description. The same for `io-wait-ratio`. This PR fixes these descriptions.. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
