-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix waged instance capacity npe on new resource #2969
base: master
Are you sure you want to change the base?
Fix waged instance capacity npe on new resource #2969
Conversation
@@ -356,6 +356,11 @@ void handleResourceCapacityCalculation(ClusterEvent event, ResourceControllerDat | |||
CurrentStateOutput currentStateOutput) { | |||
Map<String, Resource> resourceMap = event.getAttribute(AttributeName.RESOURCES.name()); | |||
if (skipCapacityCalculation(cache, resourceMap, event)) { | |||
// Ensure instance capacity is null if there are no resources. This prevents using a stale map when all resources | |||
// are removed and then a new resource is added. | |||
if (resourceMap == null || resourceMap.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we merge the 2 if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally attempted to merge the two ifs as if (resourceMap == null || resourceMap.isEmpty())
is basically a redundant check because the same check is made in skipCapacityCalculation
However, I was concerned with putting the responsibility of clearing the cache within skipCapacityCalculation because I did not want to give the method a side effect that may not be obvious to the user.
Do you have any thoughts on how to approach this to simplify the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (skipCapacityCalculation(cache, resourceMap, event) && (resourceMap == null || resourceMap.isEmpty()) ) {
cache.clearWagedCapacityProviders();
}
Issues
NPE occurs for first WAGED resource in cluster after previous WAGED resources have been removed
WagedInstanceCapacity Null Pointer Exception due to stale _instanceCapacityMap #2891
Description
Full description of the NPE can be found in the issue #2891. Here is a brief summary:
_instanceCapacityMap
is calculated with WAGED resources in cluster_instanceCapacityMap
is still present. This is only recalculated under certain conditions_instanceCapacityMap
is not recalculated before it is used. This stale_instanceCapacityMap
can lead to an NPEThis PR addresses the above issue by ensuring the
_instanceCapacityMap
is null whenever there are no WAGED resources in the cluster. This leads to the map being recalculated before it is used when a new WAGED resource is added.Tests
The following tests are written for this issue:
New test class:
TestWagedNPE
The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
Changes that Break Backward Compatibility (Optional)
N/A
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)