[FLUSS-2123][flink] Added Flink Wrapper for Histogram Statistics #2195
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a NullPointerException that occurred when Flink's Prometheus reporter attempted to access histogram statistics. The root cause was that FlinkHistogram.getStatistics() was explicitly returning null instead of wrapping the underlying Fluss histogram statistics.
Key Changes:
- Modified
FlinkHistogram.getStatistics()to return aFlinkHistogramStatisticswrapper instead of null - Added comprehensive unit tests in
FlinkHistogramTestto verify histogram functionality and statistics access
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/metrics/FlinkHistogram.java | Fixed getStatistics() method to return a FlinkHistogramStatistics wrapper instance instead of null, resolving the NPE when Flink reporters access histogram metrics |
| fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/metrics/FlinkHistogramTest.java | Added comprehensive unit tests with 9 test cases covering histogram operations including update, getCount, and all statistics methods (min, max, mean, stdDev, quantile, values, size) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wrapped.getStatistics(); | ||
|
|
||
| return null; | ||
| return new FlinkHistogramStatistics(wrapped.getStatistics()); |
There was a problem hiding this comment.
The method should handle the case where wrapped.getStatistics() returns null. While current implementations of Fluss Histogram always return a non-null HistogramStatistics, the interface doesn't explicitly guarantee this. Without a null check, this could still result in a NullPointerException when constructing FlinkHistogramStatistics if a future implementation returns null. Consider adding a null check and either throwing a descriptive exception or returning a default/empty HistogramStatistics implementation.
| return new FlinkHistogramStatistics(wrapped.getStatistics()); | |
| org.apache.fluss.metrics.HistogramStatistics stats = wrapped.getStatistics(); | |
| if (stats == null) { | |
| throw new IllegalStateException( | |
| "Underlying Fluss Histogram returned null HistogramStatistics"); | |
| } | |
| return new FlinkHistogramStatistics(stats); |
There was a problem hiding this comment.
I wasn't sure on this one. While it totally makes sense to add the additional null check, do you have a preference in terms of the null-handling behavior? Would we prefer to throw an exception? Or should be consider logging something and returning some default object?
Thoughts?
| @Test | ||
| void testGetStatisticsSize() { | ||
| testFlussHistogram.setSize(42); | ||
| HistogramStatistics statistics = flinkHistogram.getStatistics(); | ||
| assertThat(statistics.size()).isEqualTo(42); | ||
| } |
There was a problem hiding this comment.
Consider adding a test case that verifies the behavior when the wrapped Fluss histogram's getStatistics() method returns null. This would help ensure that the FlinkHistogram handles this edge case gracefully and doesn't cause a NullPointerException.
wuchong
left a comment
wuchong
left a comment
There was a problem hiding this comment.
LGTM. I rebased the branch to trigger the CI again.
2 participants
Purpose
Linked issue: close #2123
Per Issue #2123, this pull request addresses a potential null reference that could occur when histogram-related statistics were requested from Flink (as the existing statistics would be returned as explicitly null).
Brief change log
This change replaces the existing logic (i.e. returning null explicitly) and instead wraps the existing histogram statistics within an available
FlinkHistogramStatisticsinstance to ensure the underlying values can be accessed through this interface.Tests
These changes were initially reproduced within a unit test that was later transitioned into the new
FlinkHistogramTestclass which verifies the intended fix but also includes a suite of test cases to verify other histogram-related operations (e.g., accessing various statistics, etc.)API and Format
N/A
Documentation
N/A