[GitHub] [storm] Ethanlm commented on a change in pull request #3254: STORM-3596 use send assignment status in blacklist scheduling

[GitBox <gi...@apache.org>]

Ethanlm commented on a change in pull request #3254:
URL: https://github.com/apache/storm/pull/3254#discussion_r412415684



##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/blacklist/BlacklistScheduler.java
##########
@@ -266,4 +286,15 @@ private Object initializeInstance(String className, String representation) {
     public Set<String> getBlacklistSupervisorIds() {
         return Collections.unmodifiableSet(blacklistedSupervisorIds);
     }
+
+    @Override
+    public void nodeAssignmentSent(String node, boolean successful) {
+        if (!blacklistSendAssignentFailures) {
+            return;
+        }
+        if (!successful) {
+            int failCount = assignmentFailures.getOrDefault(node, 0) + 1;
+            assignmentFailures.put(node, failCount);

Review comment:
       Synchronization is needed otherwise there can be a race condition between getting the current count and updating the count (line 296 and line297)
   
   Also between `this.assignmentFailures.clear();` and `assignmentFailures.put(node, failCount);

##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/blacklist/strategies/IBlacklistStrategy.java
##########
@@ -35,7 +36,9 @@
      *       the `cluster` object.
      * @return blacklisted supervisors' id set
      */
-    Set<String> getBlacklist(List<Map<String, Set<Integer>>> badSupervisorsToleranceSlidingWindow, Cluster cluster, Topologies topologies);
+    Set<String> getBlacklist(List<Map<String, Set<Integer>>> badSupervisorsToleranceSlidingWindow,

Review comment:
       Looks like we need to modify the interface every time if we want to add another kind of failures.
   
   Technically this is a public interface and users can provide their own implementation so backwards compatibility should be preserved as long as possible. If we can make it backwards compatible, that would be fantastic.
   
   But I assume users don't really do that since this is actually tightly coupled and not really easy for users to provide their own implementation; even if they do, it is easy to make the change since the interface change is minimum. I feel this `IBlacklistStrategy` interface itself is not well defined and there are more refactoring to be done with this interface. So if it's hard to maintain backwards compatibility, this change is fine to me. Refactoring can be a follow-up

##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/blacklist/strategies/DefaultBlacklistStrategy.java
##########
@@ -72,6 +74,15 @@ public void prepare(Map<String, Object> conf) {
             }
         }
 
+        // update countMap failures for sendAssignments failing
+        for (Map<String, Integer> item : sendAssignmentFailureCount) {

Review comment:
       since we have this now, Line92 and Line 93 need to be adjusted
   
   ```
                       LOG.debug("supervisorsWithFailures : {}", supervisorsWithFailures);
                       reporter.reportBlacklist(supervisor, supervisorsWithFailures);
   ```




----------------------------------------------------------------
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:
users@infra.apache.org