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:
[email protected]
