Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils#21679
Address KAFKA-20252: Migrate clearYammerMetrics to TestUtils#21679dubeyKartikay wants to merge 2 commits intoapache:trunkfrom
Conversation
github-actions
bot
added
triage
dubeyKartikay
force-pushed
the
KAFKA-20252
branch
2 times, most recently
from
16ac865 to
1070ff7
Compare
|
@see-quick can you review this? |
see-quick
left a comment
see-quick
left a comment
There was a problem hiding this comment.
Also please change it in LogCleanerLagIntegrationTest (probably you would need to rebase). But in general I agree to having separate utils class for metrics.
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?
|
|
||
| package org.apache.kafka.server.metrics; | ||
|
|
||
| public class YammerMetricsTestUtils { |
There was a problem hiding this comment.
Could you consider renaming this to TestUtils? We will eventually migrate all methods from the Scala TestUtils to the Java one anyway
There was a problem hiding this 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
There was a problem hiding this comment.
Our plan is simply to remove the core module, so the server module is the best place for this right now.
There was a problem hiding this 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 patch has room for more code changes :) |
address checkstyle for kafka fixup add yammer metric value
dubeyKartikay
force-pushed
the
KAFKA-20252
branch
from
1070ff7 to
624e219
Compare
4 participants
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.