#2963 Fix NPE when rebalancing#2977
#2963 Fix NPE when rebalancing#2977jacob-netguardians wants to merge 7 commits intoapache:masterfrom
Conversation
In case _helixManager in one of the known controllers is _null_, finding the "leader controller" can lead to a NPE. When run as an asynchronous task, the NPE may not even be logged, and the task finishes without any trace, hence the addition of ExecutorTaskUtil class to wrap callables/runnables in such a manner that such an exception at least gets logged.
| return controllers.stream() | ||
| .filter(controller -> controller._helixManager != null) | ||
| .filter(controller -> controller._helixManager.isLeader()) | ||
| .findAny().orElse(null); |
There was a problem hiding this comment.
Are you sure using Helix format???
There was a problem hiding this comment.
Sorry, not sure I understand what you mean with "helix format". Is the formatting of the lines somehow faulty? From what I see the ".filter(...)" lines are indented the same way as the ".findAny()" line was before, so I don't get what is wrong, sorry.
There was a problem hiding this comment.
Ok, I now understood what you meant. I applied the format to all the files I touched. I don't necessarily find it more readable, but then I guess this is also a question of taste. Hopefully this is ok now.
There was a problem hiding this comment.
Please make sure you load the Helix intellij / eclipse auto code formatter.
There was a problem hiding this comment.
Done. reverted global reformatting and only applied it on the lines I had changed initially.
| } | ||
|
|
||
| public static <T> Future asyncExecute(ExecutorService service, Callable<T> task) { | ||
| public static <T> Future<T> asyncExecute(ExecutorService service, Callable<T> task) { |
There was a problem hiding this comment.
Following the boy-scout rule, every time I see something which could be easily improved, I try to do so.
Here for type-safety, it is useful for the caller of this method to know/remember that the "get" method on the future they just got would supply an object of type T instead of an object of any forgotten type.
There was a problem hiding this comment.
Let's minimize the this kind of change. The reason is that Helix release not only support java 11 but backward compatible with 8.
Any this kind of "improvement" for working code may break backward build.
There was a problem hiding this comment.
This "improvement" would have been compatible with Java 1.5, for all I know, but OK, your choice. Rolled back.
2 participants
Issues
Fixes #2963
Description
In case _helixManager in one of the known controllers is null, finding the "leader controller" can lead to a NPE.
I added a check for _helixManager member's nullity in the helix controller before using it to determine if a controller is leader or not.
When run as an asynchronous task, the NPE may not even be logged, and the task abruptly finishes without any trace, hence the addition of ExecutorTaskUtil class to wrap callables/runnables in such a manner that such an exception at least gets logged.
Tests
No added tests.
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)