[STORM-3639] Replace asserts in daemon code. #3274
Conversation
| @@ -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) { | |||
There was a problem hiding this comment.
Any reason why we don't use Preconditions here? It seems more elegant to me.
There was a problem hiding this comment.
assert/Precondition seems more concise - except for the unchecked exception being thrown.
In this specific instance (which is also throwing unchecked exception), maybe it is better to treat null map as an empty map.
There was a problem hiding this comment.
Isn't this Preconditions.checkNotNull(existingAssignments); what we wanted?
There was a problem hiding this comment.
Looked at this code more specifically. The asserts/preconditions seem superfluous.
This method is private. first check is too late, i.e. already used multiple times before being passed, and the second param is instantiated before the call.
There was a problem hiding this comment.
This null check can be kept. It can be treated as defensive programming.
What about other places? Can we use Preconditions everywhere?
There was a problem hiding this comment.
Definitely need critical review of code that is throwing unchecked exception (including AssertionError()) and look for options - or complete removal - even in the changed code.
At best, "assert" usage should be viewed as lazy coding in private methods. And should not be used in protected or public methods.
https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html (note the language that it should not be used in public methods).
This does not mean that private methods cannot throw Exceptions. They can and very often do. But those are checked exceptions (i.e. present in method signature).
The goal of this change is to remove asserts. Replace with proper checking where possible, throw checked Exception where appropriate. Reason is quite straightforward - assert statements may not be executed and gives a false impression that something is being done.
There was a problem hiding this comment.
Sorry I am a little confused. So
What's the difference between
assert existingAssignments != null && newAssignments != null;
and
assert (TopologyStatus.ACTIVE == initStatus || TopologyStatus.INACTIVE == initStatus);
Why the first one should be deleted and the second should be changed?
There was a problem hiding this comment.
First one is not required as it is never true (i.e a superfluous check in a private method - and is never activated at runtime - since it was in an assert to begin with.
Second one is a legitimate check that should be done at runtime. But was in an assert earlier and not activated earlier, but should be active.
There was a problem hiding this comment.
assertion is enabled at integration tests
https://github.com/apache/storm/blob/master/integration-test/config/storm.yaml#L39-L41
There was a problem hiding this comment.
Correct, but only in test. This is fairly standard use of assert in test. But putting this in main code is not.
If it a legitimate error condition then it should be checked. If the test needs to check something, then it should be in the test class.
Otherwise, it is gives the wrong impression that the assert is checking something at runtime, when it typically isn't.
That is the premise of this change.
bipinprasad
force-pushed
the
storm3639
branch
from
75f22a9 to
f4375c6
Compare
| @@ -514,8 +514,13 @@ private void initConfigs() { | |||
| this.topologyPriority = | |||
| ObjectReader.getInt(topologyConf.get(Config.TOPOLOGY_PRIORITY), null); | |||
|
|
|||
| assert this.topologyWorkerMaxHeapSize != null; | |||
| assert this.topologyPriority != null; | |||
| // Fails in storm-core: org.apache.storm.scheduler-test / testname: test-cluster | |||
There was a problem hiding this comment.
I would leave these asserts here and file a JIRA to address fixing the test and replacing these asserts specifically.
There was a problem hiding this comment.
There was a problem hiding this comment.
Cannot reproduce clojure test failure, closing https://issues.apache.org/jira/browse/STORM-3732
There was a problem hiding this comment.
Upon further investigation:
- assert (original code) does not cause Clojure test failure - because assert is not active at runtime
- replacement of assert - using AssertionError - (new code) will cause Clojure test failure because it is active at runtime
There was a problem hiding this comment.
I agree with @bipinprasad in this case. Assertions shouldn't be contained in production code. After rebasing our full test suite is green. Thx for the PR.
What is the purpose of the change
By default jvm disables assertion for performance, and we allow JVM options to be configured at deployment time, it is best to avoid use of assert statement and use explicit clauses to validate critical checks within storm-daemon code. Also, it would be helpful in many cases to explicitly log and proceed on failure to meet such assert expectation for debugging purpose.
How was the change tested
Rerun test suite in storm-server directory.