[#12936] Reduce TraceIndex filters using min/max values#13393
[#12936] Reduce TraceIndex filters using min/max values#13393donghun-cho merged 1 commit intopinpoint-apm:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates TraceIndex HBase scan filtering to reduce unnecessary filters by treating default elapsed ranges as “no filter”, aiming to lower filter overhead during scatter data queries.
Changes:
- Refactors
TraceIndexFilterBuilderto use separateelapsedMin/elapsedMaxwith defaults and to return aFilterList. - Skips applying an HBase
Scanfilter when the builtFilterListis empty. - Updates/extends unit tests to cover “default range => no filters” behavior and removes the now-unused elapsed range helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/scatter/dao/hbase/HbaseTraceIndexDao.java | Avoids setting an HBase scan filter when the builder returns an empty FilterList. |
| commons-server/src/test/java/com/navercorp/pinpoint/common/server/scatter/TraceIndexFilterBuilderTest.java | Updates tests to use new min/max setters and adds coverage for “no-op” filter construction. |
| commons-server/src/main/java/com/navercorp/pinpoint/common/server/scatter/TraceIndexRowKeyUtils.java | Removes the elapsed range-to-byte-list helper used by the old filtering approach. |
| commons-server/src/main/java/com/navercorp/pinpoint/common/server/scatter/TraceIndexFilterBuilder.java | Implements min/max-based filter reduction and returns a FilterList aggregating only necessary filters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static byte toElapsedByte(long elapsed) { | ||
| return fuzzyRowKeyFactory.getKey(elapsed); | ||
| } | ||
|
|
||
| public static List<Byte> toElapsedByteList(LongPair elapsedMinMax) { | ||
| return fuzzyRowKeyFactory.getRangeKey(elapsedMinMax.second(), elapsedMinMax.first()); | ||
| } | ||
|
|
||
| public static byte toErrorByte(int errorCode) { | ||
| return errorCode == 0 ? (byte) 0 : (byte) 1; | ||
| } // zero or non-zero |
There was a problem hiding this comment.
After removing toElapsedByteList(...), this file still imports LongPair and java.util.List but no longer uses them. Please remove the unused imports to keep the file clean (and to avoid failures if an unused-imports check is enabled in CI).
| public void elapsedTimeValueFilterMinMaxTest() throws IOException { | ||
| TraceIndexFilterBuilder builder = new TraceIndexFilterBuilder(); | ||
| builder.setElapsedMin(0L); | ||
| builder.setElapsedMax(Long.MAX_VALUE); // integer max is enough |
There was a problem hiding this comment.
The test sets elapsedMax(Long.MAX_VALUE) but the comment says "integer max is enough" and the production default uses Integer.MAX_VALUE. Using Integer.MAX_VALUE here (or updating the comment) would make the intent clearer and avoid relying on the special-case logic that treats values above the default as unbounded.
| builder.setElapsedMax(Long.MAX_VALUE); // integer max is enough | |
| builder.setElapsedMax((long) Integer.MAX_VALUE); // integer max is enough |
| private List<Filter> createElapsedValueFilter(long elapsedMin, long elapsedMax) { | ||
| List<Filter> filters = new ArrayList<>(); | ||
| if (elapsedMin > DEFAULT_ELAPSED_MIN) { | ||
| int yLow = (int) elapsedMin; | ||
| filters.add(new SingleColumnValueFilter(INDEX.getName(), INDEX.getName(), CompareOperator.GREATER_OR_EQUAL, new BinaryPrefixComparator(Bytes.toBytes(yLow)))); | ||
| } | ||
| if (elapsedMax < DEFAULT_ELAPSED_MAX) { | ||
| int yHigh = (int) elapsedMax; |
There was a problem hiding this comment.
setElapsedMin/Max() accept long, but createElapsedValueFilter() casts them to int without bounds checking. If a caller passes a value > Integer.MAX_VALUE, the cast can overflow and produce an incorrect HBase value filter; consider validating/clamping to [0, Integer.MAX_VALUE] or changing the API to accept int to match the stored value type.
| private List<Filter> createElapsedValueFilter(long elapsedMin, long elapsedMax) { | |
| List<Filter> filters = new ArrayList<>(); | |
| if (elapsedMin > DEFAULT_ELAPSED_MIN) { | |
| int yLow = (int) elapsedMin; | |
| filters.add(new SingleColumnValueFilter(INDEX.getName(), INDEX.getName(), CompareOperator.GREATER_OR_EQUAL, new BinaryPrefixComparator(Bytes.toBytes(yLow)))); | |
| } | |
| if (elapsedMax < DEFAULT_ELAPSED_MAX) { | |
| int yHigh = (int) elapsedMax; | |
| private int clampElapsedToInt(long elapsed) { | |
| if (elapsed < 0L) { | |
| return 0; | |
| } | |
| if (elapsed > Integer.MAX_VALUE) { | |
| return Integer.MAX_VALUE; | |
| } | |
| return (int) elapsed; | |
| } | |
| private List<Filter> createElapsedValueFilter(long elapsedMin, long elapsedMax) { | |
| List<Filter> filters = new ArrayList<>(); | |
| if (elapsedMin > DEFAULT_ELAPSED_MIN) { | |
| int yLow = clampElapsedToInt(elapsedMin); | |
| filters.add(new SingleColumnValueFilter(INDEX.getName(), INDEX.getName(), CompareOperator.GREATER_OR_EQUAL, new BinaryPrefixComparator(Bytes.toBytes(yLow)))); | |
| } | |
| if (elapsedMax < DEFAULT_ELAPSED_MAX) { | |
| int yHigh = clampElapsedToInt(elapsedMax); |
| private List<Filter> createElapsedByteRowFilter(long elapsedMin, long elapsedMax) { | ||
| if (elapsedMin <= DEFAULT_ELAPSED_MIN && elapsedMax >= DEFAULT_ELAPSED_MAX) { | ||
| return List.of(); | ||
| } | ||
| Byte minByte = null; | ||
| Byte maxByte = null; | ||
| if (elapsedMin > DEFAULT_ELAPSED_MIN) { | ||
| minByte = TraceIndexRowKeyUtils.toElapsedByte(elapsedMin); | ||
| } | ||
| if (elapsedMax < DEFAULT_ELAPSED_MAX) { | ||
| maxByte = TraceIndexRowKeyUtils.toElapsedByte(elapsedMax); | ||
| } | ||
|
|
There was a problem hiding this comment.
createElapsedByteRowFilter(elapsedMin, elapsedMax) assumes elapsedMin <= elapsedMax. Previously getRangeKey() normalized high/low internally; with the new separate setters it’s easy for callers to set them in the wrong order, producing contradictory >=/<= filters. Consider normalizing (swap) inside build()/createElapsed*Filter() (or enforce via setters) to keep behavior robust.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13393 +/- ##
=========================================
Coverage 33.10% 33.10%
+ Complexity 10975 10974 -1
=========================================
Files 4066 4066
Lines 94350 94371 +21
Branches 9817 9819 +2
=========================================
+ Hits 31232 31244 +12
- Misses 60437 60440 +3
- Partials 2681 2687 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
donghun-cho
force-pushed
the
FilterBuilder
branch
from
cd3a5e5 to
ec6575b
Compare
|
1 participant
No description provided.