[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r491041226
##
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
existingAssignments,
Map
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {
Review comment:
assertion is enabled at integration tests
https://github.com/apache/storm/blob/master/integration-test/config/storm.yaml#L39-L41
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r435449649
##
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
existingAssignments,
Map
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {
Review 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?
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
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
existingAssignments,
Map
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r434002985
##
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
existingAssignments,
Map
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {
Review comment:
This null check can be kept. It can be treated as defensive programming.
What about other places? Can we use Preconditions everywhere?
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r434002985
##
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
existingAssignments,
Map
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {
Review comment:
What about other places? Can we use Preconditions everywhere?
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r433415263
##
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
existingAssignments,
Map
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {
Review comment:
Isn't this `Preconditions.checkNotNull(existingAssignments);` what we
wanted?
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.
Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r431540155
##
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
existingAssignments,
Map
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {
Review comment:
Any reason why we don't use `Preconditions` here? It seems more elegant
to me.
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
