[refactor] Decouple pinot-common test deps with FakeMetrics + plugin subclass tests; add SettableValue.getValue()

[xiangfu0]

Summary

Untangle the circular test-scope dependency where pinot-common depended on pinot-yammer and pinot-dropwizard for its own tests. Plugin-specific tests move into their plugin modules; shared AbstractMetrics test logic stays in pinot-common and runs against an in-memory fake registry, with each plugin subclassing the base suite for its own registry.

Why

pinot-common/pom.xml pulled in pinot-yammer and pinot-dropwizard at <scope>test</scope> solely because a handful of tests under pinot-common/src/test/.../metrics/ exercised plugin-specific implementations. This is backwards — plugin modules should depend on pinot-common, not the other way around — and it blocked follow-up work to make pinot-common self-contained.

What changed

pinot-common

• Dropped pinot-yammer and pinot-dropwizard test-scope deps; removed the surefire excludes that were carrying Dropwizard prometheus tests.
• New: public AbstractMetrics.getMetricPrefix() getter so test helpers can compose a PinotMetricName without reaching into the protected field (or an instanceof cascade).
• Refactored: AbstractMetricsTest is now an abstract base with hooks metricsFactoryClassName(), buildRegistry(), createInspector(), getGaugeValue(). Every @Test stays generic.
• Refactored: MetricsInspector is now an abstract class (lastMetric, getTimerSumMs, getMeteredCount). Plugin-specific concrete subclasses live in each plugin module.
• Refactored: MetricValueUtils no longer casts to YammerSettableGauge — routes through AbstractMetrics.getGaugeValue() so it's implementation-agnostic.
• Refactored: PinotMetricUtilsTest now bootstraps with the new FakeMetricsFactory instead of assuming Yammer on the classpath.
• Refactored: ConnectionFailureDetectorTest constructs NoopPinotMetricsRegistry directly, removing its dependence on ServiceLoader-discovered factories.
• New test harness under pinot-common/src/test/java/org/apache/pinot/plugin/metrics/fake/:
  • FakePinotMetricsRegistry — in-memory, with listener replay semantics per the SPI contract.
  • FakeMetricsFactory — no JMX reporter (intentional, to avoid cross-test MBean collisions).
  • Fake wrapper types: FakePinotGauge/Meter/Counter/Timer, FakePinotMetricName, plus FakeMetricsInspector.
  • FakeMetricsAbstractMetricsTest — concrete subclass that runs the base suite against the fake registry.
• JMX lifecycle: @AfterClass PinotMetricUtils.cleanUp() on both AbstractMetricsTest and PinotMetricUtilsTest so the static factory is torn down between test classes.

pinot-plugins/pinot-metrics/pinot-yammer

• Added pinot-common + pinot-common:test-jar at test scope so the plugin module can extend the shared tests.
• Relocated the four Yammer prometheus tests from pinot-common/src/test/.../prometheus/yammer/ into the plugin module.
• Renamed yammer's MetricValueUtils → YammerMetricValueUtils to avoid an FQN clash with the impl-agnostic MetricValueUtils that stays in pinot-common.
• YammerMetricsInspector moved next to the real YammerMetricsRegistry.
• YammerAbstractMetricsTest subclasses the shared AbstractMetricsTest with a real YammerMetricsRegistry and uses YammerMetricValueUtils for gauge reads.

pinot-plugins/pinot-metrics/pinot-dropwizard

• Added pinot-common:test-jar at test scope.
• Relocated the four Dropwizard prometheus tests into the plugin module. Preserved @Test(enabled = false) on the ones that were disabled, and fixed a latent copy-paste bug in DropwizardServerPrometheusMetricsTest.getPinotMetricsFactory() that was returning YammerMetricsFactory.
• New DropwizardMetricValueUtils mirrors the Yammer helper.
• New DropwizardMetricsInspector — Timer sum derived from snapshot.getValues() summation (Dropwizard's Timer has no sum() method).
• DropwizardAbstractMetricsTest subclasses the shared AbstractMetricsTest.

Behavior notes

• MetricValueUtils behavior change: now routes through AbstractMetrics.getGaugeValue(), which only reads the internal _gaugeValues map. For gauges registered via pure supplier paths (setOrUpdateGauge(Supplier) / setOrUpdateGauge(long) / addCallbackGauge), _gaugeValues is not populated — so MetricValueUtils.getGaugeValue returns 0 and gaugeExists returns false. Upstream Fix flaky MergeRollupMinionClusterIntegrationTest gauge assertions #18260 already landed the required adjustment for MergeRollupMinionClusterIntegrationTest; other test consumers reading supplier-registered gauges via MetricValueUtils should switch to Yammer/DropwizardMetricValueUtils or read from the registry directly.
• No production behavior change other than the new AbstractMetrics.getMetricPrefix() getter.

Test plan

• ./mvnw -pl pinot-common test -Dtest=FakeMetricsAbstractMetricsTest — 15/15 pass
• ./mvnw -pl pinot-common test -Dtest=PinotMetricUtilsTest — 6/6 pass
• ./mvnw -pl pinot-common test -Dtest=ConnectionFailureDetectorTest — 4/4 pass
• ./mvnw -pl pinot-plugins/pinot-metrics/pinot-yammer test — all pass (incl. YammerAbstractMetricsTest 15/15 + prometheus suites)
• ./mvnw -pl pinot-plugins/pinot-metrics/pinot-dropwizard test — all pass (incl. DropwizardAbstractMetricsTest 15/15)
• ./mvnw -pl pinot-controller test -Dtest=SegmentStatusCheckerTest — 23/23 pass (external consumer of MetricValueUtils)
• ./mvnw spotless:check checkstyle:check license:check -pl pinot-common,pinot-plugins/pinot-metrics/pinot-yammer,pinot-plugins/pinot-metrics/pinot-dropwizard — clean

[Copilot]

Pull request overview

This PR relocates Yammer/Dropwizard metrics implementation tests out of pinot-common and into their respective metrics plugin modules to remove backwards (test-scope) dependencies from pinot-common onto plugin artifacts, while also adjusting a few test utilities and metrics behaviors to keep coverage intact.

Changes:

• Moved Yammer + Dropwizard Prometheus metrics tests into pinot-yammer and pinot-dropwizard test sources, and updated packages/paths accordingly.
• Updated plugin POMs to depend on pinot-common (including test-jar) for shared test bases, and removed plugin test dependencies + old test excludes from pinot-common.
• Refactored metrics test utilities (MetricValueUtils / new YammerMetricValueUtils) and updated ConnectionFailureDetectorTest to avoid requiring plugin discovery.

Reviewed changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerServerPrometheusMetricsTest.java	Package move + config path update for server Prometheus metrics test.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerMinionPrometheusMetricsTest.java	Package move + config path update for minion Prometheus metrics test.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerControllerPrometheusMetricsTest.java	Package move + config path update for controller Prometheus metrics test.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/prometheus/YammerBrokerPrometheusMetricsTest.java	Package move + config path update for broker Prometheus metrics test.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/plugin/metrics/yammer/YammerMetricValueUtils.java	New Yammer-specific gauge-reading helper for moved tests.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/PinotMetricUtilsTest.java	Moved PinotMetricUtils tests into plugin module test sources.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/MetricsInspector.java	Moved registry inspection helper into plugin module test sources.
pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/AbstractMetricsTest.java	Moved + adjusted AbstractMetrics tests to use Yammer-specific gauge value utility.
pinot-plugins/pinot-metrics/pinot-yammer/pom.xml	Adds pinot-common + test-jar and other test deps needed by moved tests.
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardServerPrometheusMetricsTest.java	Package move + corrected factory usage to Dropwizard.
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardMinionPrometheusMetricsTest.java	Package move for minion dropwizard Prometheus test (still disabled).
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardControllerPrometheusMetricsTest.java	Package move for controller dropwizard Prometheus test (still disabled).
pinot-plugins/pinot-metrics/pinot-dropwizard/src/test/java/org/apache/pinot/plugin/metrics/dropwizard/prometheus/DropwizardBrokerPrometheusMetricsTest.java	Package move for broker dropwizard Prometheus test (still disabled).
pinot-plugins/pinot-metrics/pinot-dropwizard/pom.xml	Adds pinot-common + test-jar and other test deps needed by moved tests.
pinot-common/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java	Refactors test helper to read via AbstractMetrics.getGaugeValue() (implementation-agnostic).
pinot-common/src/test/java/org/apache/pinot/common/failuredetector/ConnectionFailureDetectorTest.java	Switches to NoopPinotMetricsRegistry to avoid requiring plugin-based registry initialization in this unit test.
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java	Attempts to add supplier-gauge fallback behavior to getGaugeValue().
pinot-common/pom.xml	Removes plugin test deps and old surefire excludes now that tests moved to plugin modules.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.52%. Comparing base (159f046) to head (f87a966).

Files with missing lines	Patch %	Lines
...not/plugin/metrics/yammer/YammerSettableGauge.java	0.00%	1 Missing ⚠️
.../java/org/apache/pinot/spi/metrics/PinotGauge.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18259      +/-   ##
============================================
+ Coverage     63.50%   63.52%   +0.01%     
- Complexity     1627     1658      +31     
============================================
  Files          3244     3244              
  Lines        197367   197370       +3     
  Branches      30540    30540              
============================================
+ Hits         125347   125384      +37     
+ Misses        61983    61945      -38     
- Partials      10037    10041       +4     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-11	63.50% <33.33%> (+0.03%)	⬆️
java-21	63.50% <33.33%> (+0.03%)	⬆️
temurin	63.52% <33.33%> (+0.01%)	⬆️
unittests	63.52% <33.33%> (+0.01%)	⬆️
unittests1	55.53% <33.33%> (+0.04%)	⬆️
unittests2	34.99% <33.33%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

pinot-plugins/pinot-metrics/pinot-yammer/src/test/java/org/apache/pinot/common/metrics/AbstractMetricsTest.java:51

• buildTestMetrics() calls PinotMetricUtils.init(config) every time it's invoked. PinotMetricUtils.init() registers new MetricsRegistryRegistrationListener instances into a static map and doesn't clear them on cleanUp(), so repeated init calls across test methods/classes can accumulate listeners and create cross-test interference. Consider initializing PinotMetricUtils once (e.g., in a @BeforeClass) and reusing it for the whole test class, or avoiding init() here if it isn't required for these tests.

[Jackie-Jiang]

This change is touching production class. If the change is required, please revise the PR title and description

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.