Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r435339674
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -814,7 +814,12 @@ private static int numUsedWorkers(SchedulerAssignment
assignment) {
private boolean auditAssignmentChanges(Map<String, Assignment>
existingAssignments,
Map<String, Assignment>
newAssignments) {
- assert existingAssignments != null && newAssignments != null;
+ if (existingAssignments == null) {
Review comment:
Your second point about defensive programming kinda makes sense to me.
For the first point, "no reason to throw unchecked exceptions in a private
method because it is totally in control of the class scope", I don't disagree
with that. But with this assumption/standard, there are changes in this PR that
can be removed too since some of them are also this case, for example,
`if (TopologyStatus.ACTIVE != initStatus && TopologyStatus.INACTIVE !=
initStatus) `.
And I think those places are where we can use `assert`. My understanding of
using `assert` is to detect mistakes developers make during development. Since
we have total control of a private method, we don't expect those `null` or
invalid case to really happen. So we detect those in development, but not in
production.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
