You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "dubeyKartikay (via GitHub)" <gi...@apache.org> on 2026/03/08 16:21:26 UTC
[PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
dubeyKartikay opened a new pull request, #21679:
URL: https://github.com/apache/kafka/pull/21679
In this change, I had to remove the references of kafka.utils.TestUtils.clearYammerMetrics (Scala version) from Java tests.
There was already a Java implementation written in server/src/test/java/org/apache/kafka/server/share/fetch/ShareFetchTestUtils.java, so my job was made a lot easier.
Instead of directly importing this into all the affected modules, I considered moving clearYammerMetrics to a more general class like TestUtils.java.
But IMO, this function is too specific to be in TestUtils.java. So I made a new YammerMetricsTestUtils class that could deal with all the util functions for the Yammer related things.
In this PR, I have only changed the clearYammerMetrics.
I can see another Yammer-related function that could belong in YammerMetricsTestUtils, namely yammerMetricValue. But that change should be taken as a separate PR. All my opinion of course; I'm open to feedback.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #21679:
URL: https://github.com/apache/kafka/pull/21679#discussion_r3043962411
##########
server-common/src/test/java/org/apache/kafka/server/util/ServerTestUtils.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.server.util;
+
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import com.yammer.metrics.core.Gauge;
+
+public final class ServerTestUtils {
+
+ /**
+ * Clear all the yammer metrics.
+ */
+ public static void clearYammerMetrics() {
+ KafkaYammerMetrics.defaultRegistry().allMetrics().keySet().forEach(
+ metricName -> KafkaYammerMetrics.defaultRegistry().removeMetric(metricName));
+ }
+
+ /**
+ * Fetch the gauge value from the yammer metrics.
+ *
+ * @param name The name of the metric.
+ * @return The gauge value as a number.
+ */
+ public static Number yammerMetricValue(String name) {
+ Gauge gauge = (Gauge) KafkaYammerMetrics.defaultRegistry().allMetrics().entrySet().stream()
Review Comment:
We can use `Gauge<?> gauge = (Gauge<?>) KafkaYammerMetrics.defaultRegistry()....` to get rid of the raw uses warnings.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4019429955
@see-quick can you review this?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on code in PR #21679:
URL: https://github.com/apache/kafka/pull/21679#discussion_r2913829231
##########
server/src/test/java/org/apache/kafka/server/metrics/YammerMetricsTestUtils.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.server.metrics;
+
+public class YammerMetricsTestUtils {
Review Comment:
Hey, sorry for the noise, but i ran into an issue while trying to rename `YammerMetricsTestUtils` to `TestUtils`.
Some of the test files already import a `TestUtils` java class from the `clients` module, hence we have a name clash.
Should we rename this to `ServerTestUtils` instead?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4227161566
Thanks for the review, Michael. I checked and we can鈥檛 delete it as-is yet because there are still Scala tests calling TestUtils.clearYammerMetrics().
Should I migrate those Scala tests to ServerTestUtils.clearYammerMetrics() in this PR and then remove the Scala 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4237008941
> Should I migrate those Scala tests to ServerTestUtils.clearYammerMetrics() in this PR and then remove the Scala method?
Yes, I think we can do it as part of this PR.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4041022429
@dubeyKartikay would you please fix the conflicts?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4157388911
Thanks for the feedback!
> I suspect those references are in clients-integration-tests?
Yup, You're right, i misspoke
Moved `ServerTestUtils` from the `server` module to `server-common`, since `server-common` already owns `KafkaYammerMetrics `and is a dependency of all modules that need the test
utility (server, storage, core, clients-integration-tests). This also let me remove the testImplementation project(':server').sourceSets.test.output lines I had added to `build.gradle`
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-20252 Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #21679:
URL: https://github.com/apache/kafka/pull/21679
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4106146615
@dubeyKartikay Good catch on the dependency issues. Actually, the `clients` module itself should not be using `clearYammerMetrics` at all. I suspect those references are in `clients-integration-tests`?
Regarding the visibility issue for the `storage` module, you are right that keeping it in the server module makes thing tricky. A much more suitable place for this utility would be the server-common module. WDYT?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4113577448
> A much more suitable place for this utility would be the server-common module. WDYT?
+1
We're starting to have `TestUtils` classes in all modules and it's not very clear how they overlap or complete each other. I've seen some tests importing 2 different `TestUtils` classes. This is not a problem to resolve in this PR, this will need its dedicated issue. It's just something to keep in mind 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on code in PR #21679:
URL: https://github.com/apache/kafka/pull/21679#discussion_r2906967165
##########
server/src/test/java/org/apache/kafka/server/metrics/YammerMetricsTestUtils.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.server.metrics;
+
+public class YammerMetricsTestUtils {
Review Comment:
If we are planning on migrating all of TestUtils.scala into this file, should we also move this file from the `server` module to the `core` module? under `core/src/test/java/kafka/utils` as TestUtils.java
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4114551564
> We're starting to have TestUtils classes in all modules and it's not very clear how they overlap or complete each other. I've seen some tests importing 2 different TestUtils classes. This is not a problem to resolve in this PR, this will need its dedicated issue. It's just something to keep in mind here.
opened https://issues.apache.org/jira/browse/KAFKA-20350
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on code in PR #21679:
URL: https://github.com/apache/kafka/pull/21679#discussion_r3078117700
##########
server-common/src/test/java/org/apache/kafka/server/util/ServerTestUtils.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.server.util;
+
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import com.yammer.metrics.core.Gauge;
+
+public final class ServerTestUtils {
+
+ /**
+ * Clear all the yammer metrics.
+ */
+ public static void clearYammerMetrics() {
+ KafkaYammerMetrics.defaultRegistry().allMetrics().keySet().forEach(
+ metricName -> KafkaYammerMetrics.defaultRegistry().removeMetric(metricName));
+ }
+
+ /**
+ * Fetch the gauge value from the yammer metrics.
+ *
+ * @param name The name of the metric.
+ * @return The gauge value as a number.
+ */
+ public static Number yammerMetricValue(String name) {
+ Gauge gauge = (Gauge) KafkaYammerMetrics.defaultRegistry().allMetrics().entrySet().stream()
Review Comment:
addressed
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4242383618
deleted clearYammerMetrics from TestUtils.java, changed Scala imports to use the java version.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4189187632
Checks pass 馃コ
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4023946720
> I can see another Yammer-related function that could belong in YammerMetricsTestUtils, namely yammerMetricValue.
Yes, it would be better in separate PR but maybe @chia7712 has different view?
this patch has room for more code 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "dubeyKartikay (via GitHub)" <gi...@apache.org>.
dubeyKartikay commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4042453103
Couple of things i want to point out.
1. I had to make the server module test files visible to `clients` and `storage` module.
2. Had to edit checkstyle xml to allow imports from org.kafka.server.utils in `purgatory` submodule in storage.
Both these seem icky to me, but the alternative was to duplicate code.
What are your thoughts?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21679:
URL: https://github.com/apache/kafka/pull/21679#discussion_r2906999309
##########
server/src/test/java/org/apache/kafka/server/metrics/YammerMetricsTestUtils.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.server.metrics;
+
+public class YammerMetricsTestUtils {
Review Comment:
Our plan is simply to remove the core module, so the server module is the best place for this right 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21679:
URL: https://github.com/apache/kafka/pull/21679#discussion_r2905554970
##########
server/src/test/java/org/apache/kafka/server/metrics/YammerMetricsTestUtils.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.server.metrics;
+
+public class YammerMetricsTestUtils {
Review Comment:
Could you consider renaming this to `TestUtils`? We will eventually migrate all methods from the Scala `TestUtils` to the Java one anyway
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #21679:
URL: https://github.com/apache/kafka/pull/21679#issuecomment-4163693449
There's a checkstyle failure:
```
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':storage:spotlessJavaCheck'.
> The following files had format violations:
src/test/java/org/apache/kafka/server/purgatory/DelayedRemoteListOffsetsTest.java
@@ -24,8 +24,8 @@
import路org.apache.kafka.common.record.internal.FileRecords;
import路org.apache.kafka.common.requests.ListOffsetsResponse;
import路org.apache.kafka.server.metrics.KafkaYammerMetrics;
+import路org.apache.kafka.server.util.ServerTestUtils;
import路org.apache.kafka.server.util.timer.MockTimer;
-import路org.apache.kafka.server.util.ServerTestUtils;
import路org.apache.kafka.storage.internals.log.AsyncOffsetReadFutureHolder;
import路org.apache.kafka.storage.internals.log.OffsetResultHolder;
Run './gradlew spotlessApply' to fix all violations.
```
See https://github.com/apache/kafka/actions/runs/23761985994/job/69358115728?pr=21679
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org