Fix NPE in ServerGrpcChannelBackoffResetHandler#18139
Fix NPE in ServerGrpcChannelBackoffResetHandler#18139timothy-e wants to merge 7 commits intoapache:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18139 +/- ##
============================================
- Coverage 64.01% 63.13% -0.88%
+ Complexity 1617 1616 -1
============================================
Files 3192 3213 +21
Lines 193936 195733 +1797
Branches 29937 30242 +305
============================================
- Hits 124140 123584 -556
- Misses 59990 62275 +2285
- Partials 9806 9874 +68
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Both require a full scan to rebuild _shuttingDownServers from the current cluster state. | ||
| NotificationContext.Type type = context.getType(); | ||
| if (type == NotificationContext.Type.INIT || context.getIsChildChange()) { | ||
| String pathChanged = context.getPathChanged(); |
There was a problem hiding this comment.
We should dig deeper. Do you see a code path in Helix that can result in null pathChanges?
There was a problem hiding this comment.
I found the bug and when it was introduced. Updated PR description and the change.
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Nice catch on the root cause
| if (type == NotificationContext.Type.INIT || context.getIsChildChange()) { | ||
| handleFullScan(); | ||
| } else { | ||
| } else if (type == NotificationContext.Type.CALLBACK) { |
There was a problem hiding this comment.
Add some comments about why we need this check to prevent someone accidentally removing it again
| if (pathChanged == null) { | ||
| // Shouldn't happen, but be defensive. | ||
| return; | ||
| } |
There was a problem hiding this comment.
This means missing a callback. Suggest not making it defensive. I don't think this can happen
4 participants
Pinot integration tests failed flakily with
getPathChangedcan returnnullwhen the notification type isFINALIZE. This was originally present in the PR that introduced this file, but removed because it was accidentally viewed as redundant.Also, add a defensive null check before using the
pathChanged, since it isn't guaranteed thatpathChangedwill be non-null.