[GitHub] [helix] subprotocol edited a comment on issue #1888: Ephemeral Instance Support

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

subprotocol edited a comment on issue #1888:
URL: https://github.com/apache/helix/issues/1888#issuecomment-962260090


   @jiajunwang I agree, a purge instance criteria builder is a great idea here. Below is a rough sketch of the required change as I see things.
   
   
   ```java
   // Add criteria builder
   public class HelixPurgeInstanceCriteriaBuilder {
     public HelixPurgeInstanceCriteriaBuilder withSecondsOffline(int seconds);
     public HelixPurgeInstanceCriteriaBuilder withTags(Set<String> tags);
     public HelixPurgeInstanceCriteria build();
   }
   
   // Add purge instance criteria
   public class HelixPurgeInstanceCriteria {
     int getSecondsOffline();
     Set<String> getTags();
   }
   ```
   
   **HelixAdmin:**
   ```java
   // Add to HelixAdmin interface
   void purgeOfflineInstances(String clusterName, HelixPurgeInstanceCriteria purgeCriteria);
   ```
   
   **ZkHelixAdmin:**
   ```java
   // Override and implement in ZkHelixAdmin
   void purgeOfflineInstances(String clusterName, HelixPurgeInstanceCriteria purgeCriteria);
   
   // For backwards compatibility—keep old purgeOfflineInstances method signature as-is, replace the
   // implementation with new builder pattern that calls purgeOfflineInstances(clusterName, purgeCriteria)
   ```
   
   
   Example Usages:
   ```java
   // Purge instances that have been offline for at least 1 hour and tagged
   // with both: "worker" and "spot-instance"
   HelixPurgeInstanceCriteria purgeCriteria = new HelixPurgeInstanceCriteriaBuilder()
     .withSecondsOffline(3600)
     .withTags(Set.of("worker", "spot-instance"))
     .build();
   
   admin.purgeOfflineInstances("MyCluster", purgeCriteria);
   ```
   
   ```java
   // Purge instances that have been offline for 1 day
   HelixPurgeInstanceCriteria purgeCriteria = new HelixPurgeInstanceCriteriaBuilder()
     .withSecondsOffline(86400)
     .build();
   
   admin.purgeOfflineInstances("MyCluster", purgeCriteria);
   ```
   
   ```java
   // Helix does not allow dropping instances that are online
   HelixPurgeInstanceCriteria purgeCriteria = new HelixPurgeInstanceCriteriaBuilder()
     .build(); // Throws exception with message stating that withSecondsOffline is required
   ```
   
   
   
   **Clarifying Point**
   `long offlineDuration` is currently used on purgeOfflineInstances; It's not clear on the current method signature or docs that this is in milliseconds. I thought `withSecondsOffline` would be more clear and intuitive.
   
   
   **Drop vs. Purge**
   HelixAdmin currently has these methods for dropping/purging:
   * `void dropInstance(String clusterName, InstanceConfig instanceConfig);` - singular, fails if instance is online
   * `void purgeOfflineInstances(String clusterName, long offlineDuration);` - plural, only applied to offline instances
   
   Though dropping a live instance is not supported now, in the future it could be?  If so `dropInstances(clusterName, dropCriteria)` may make more sense than `purgeOfflineInstances(clusterName, purgeCriteria)`.  In this world, if/when Helix supports dropping live instances, the builder would no longer throw an exception when withSecondsOffline isn't specified.
   
   In my mind this feels more future proof.  I wanted to float this idea out there.  If this makes more sense I can refine the example code above.
   
   
   I welcome any suggestions.  Once aligned, would a PR be a sufficient starting point? Or should a JIRA be created to track this?


-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org