[ISSUE #10620] Fix NPE when recording RPC latency for local proxy requests#10621
[ISSUE #10620] Fix NPE when recording RPC latency for local proxy requests#10621majialoong wants to merge 2 commits into
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes NPE in NettyRemotingAbstract.writeResponse when recording RPC latency for local proxy requests by initializing processTimer in LocalRemotingCommand.createRequestCommand().
Findings
- [Info]
LocalRemotingCommand.java:35—Stopwatch.createStarted()is the correct initialization, matching the pattern used byRemotingCommandafterNettyDecoderprocessing. Clean and minimal fix. - [Info]
LocalRemotingCommandTest.java— Good test coverage with two focused tests: one verifying timer initialization, another verifying the full latency recording flow throughwriteResponse. The use ofNopLongHistogramoverride andSimpleChannelstub is appropriate.
Suggestions
- Consider adding a brief comment in
LocalRemotingCommand.createRequestCommand()explaining why the timer is initialized here (i.e., local commands bypassNettyDecoder), to help future maintainers understand the context.
Verdict
✅ Approve — Correct fix for a real NPE bug. The change is minimal, well-targeted, and includes proper test coverage. No compatibility or performance concerns.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10621 +/- ##
=============================================
- Coverage 48.31% 48.21% -0.10%
+ Complexity 13480 13446 -34
=============================================
Files 1380 1380
Lines 101009 101010 +1
Branches 13080 13080
=============================================
- Hits 48799 48707 -92
- Misses 46258 46321 +63
- Partials 5952 5982 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Follow-up)
Thanks for addressing the suggestion! The added comment in LocalRemotingCommand.createRequestCommand() clearly explains why the timer is initialized there.
Changes in a6016f94
- Added inline comment explaining that local requests bypass
NettyDecoder— exactly as suggested.
Verdict
✅ Looks good. No further concerns.
Which Issue(s) This PR Fixes
Brief Description
Initialize
processTimerforLocalRemotingCommandto prevent an NPE and ensure RPC latency metrics are recorded correctly.How Did You Test This Change?
Added unit tests to verify that the timer is initialized and RPC latency can be recorded successfully.