narendly commented on a change in pull request #1494:
URL: https://github.com/apache/helix/pull/1494#discussion_r514898910
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -250,6 +251,13 @@ public void dropInstance(String clusterName,
InstanceConfig instanceConfig) {
}
}
+ @Override
+ public void purgeOfflineInstances(String clusterName, Long timeout) {
+ Map<String, InstanceConfig> timeoutOfflineInstances =
findTimeoutOfflineInstances(clusterName
+ , timeout);
+ timeoutOfflineInstances.values().forEach(instance ->
dropInstance(clusterName, instance));
Review comment:
two high-level comments:
1. are we not using the lock? as discussed offline, i think to complete this
feature, it would be a good idea to use the distributed lock here since all
sorts of race conditions are possible
2. what kind of failure handling are we doing in this call? we're making
multiple dropInstance calls here and there's no guarantee that all would go
through. plus, the method itself isn't returning anything so there's no way for
the user to determine whether the purge operation was successful or not
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
