[#noissue] Inject StringAllocatorFactory into the span row mapper#14061
Merged
Conversation
SpanMapperV2 created CachedStringAllocator from a raw cacheSize int per row. Introduce StringAllocatorFactory owning the per-scope allocation strategy: the property binding moves to TraceConfiguration, and the mapper just calls create() per row. The factory contract documents why a caching allocator must not outlive a single row decode (its keys wrap the row's backing array).
|
There was a problem hiding this comment.
Pull request overview
This PR refactors trace span row decoding to use an injected StringAllocatorFactory, centralizing the string-allocation/caching strategy in Spring configuration and ensuring allocators are created per row decode scope (avoiding cache key lifetime issues tied to row backing arrays).
Changes:
- Introduces
StringAllocatorFactoryincommons-bufferto create per-scopeStringAllocatorinstances (default vs LRU-cached). - Moves
web.hbase.mapper.cache.string.sizeproperty binding intoTraceConfigurationand exposes aStringAllocatorFactorybean. - Updates
SpanMapperV2andSpanMapperFactoryto use the factory rather than constructingCachedStringAllocatordirectly from anint.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/trace/TraceConfiguration.java | Adds a StringAllocatorFactory bean wired from web.hbase.mapper.cache.string.size. |
| web/src/main/java/com/navercorp/pinpoint/web/trace/dao/mapper/SpanMapperV2.java | Replaces per-row cacheSize logic with stringAllocatorFactory.create() per row. |
| web/src/main/java/com/navercorp/pinpoint/web/trace/dao/mapper/SpanMapperFactory.java | Injects StringAllocatorFactory and passes it into SpanMapperV2 constructors. |
| commons-buffer/src/main/java/com/navercorp/pinpoint/common/buffer/StringAllocatorFactory.java | Adds the new factory interface and implementations for default vs cached allocators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+39
| static StringAllocatorFactory cached(int cacheSize) { | ||
| return new CachedStringAllocatorFactory(cacheSize); | ||
| } |
Comment on lines
+19
to
+23
| /** | ||
| * Creates a {@link StringAllocator} per decode scope. Callers must call | ||
| * {@link #create()} once per scope (e.g. per row) and not share the returned | ||
| * allocator across scopes: a caching allocator keys on buffers over the | ||
| * decoded data's backing array, so it must not outlive that array. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #14061 +/- ##
============================================
- Coverage 34.71% 34.70% -0.02%
+ Complexity 12357 12353 -4
============================================
Files 4205 4206 +1
Lines 99972 99984 +12
Branches 10720 10720
============================================
- Hits 34707 34699 -8
- Misses 62321 62338 +17
- Partials 2944 2947 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SpanMapperV2 created CachedStringAllocator from a raw cacheSize int per
row. Introduce StringAllocatorFactory owning the per-scope allocation
strategy: the property binding moves to TraceConfiguration, and the
mapper just calls create() per row. The factory contract documents why
a caching allocator must not outlive a single row decode (its keys wrap
the row's backing array).