zpinto commented on code in PR #2986:
URL: https://github.com/apache/helix/pull/2986#discussion_r1924255760


##########
helix-rest/src/main/java/org/apache/helix/rest/server/service/VirtualTopologyGroupService.java:
##########
@@ -60,15 +62,15 @@ public class VirtualTopologyGroupService {
   private final ClusterService _clusterService;
   private final ConfigAccessor _configAccessor;
   private final HelixDataAccessor _dataAccessor;
-  private final VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
+  private VirtualGroupAssignmentAlgorithm _assignmentAlgorithm;
 
   public VirtualTopologyGroupService(HelixAdmin helixAdmin, ClusterService 
clusterService,
       ConfigAccessor configAccessor, HelixDataAccessor dataAccessor) {
     _helixAdmin = helixAdmin;
     _clusterService = clusterService;
     _configAccessor = configAccessor;
     _dataAccessor = dataAccessor;
-    _assignmentAlgorithm = FifoVirtualGroupAssignmentAlgorithm.getInstance();
+    _assignmentAlgorithm = 
FaultZoneBasedVirtualGroupAssignmentAlgorithm.getInstance(); // default 
assignment algorithm

Review Comment:
   We should not change the algorithm default or this will break backwards 
compatibility.



##########
helix-core/src/main/java/org/apache/helix/cloud/topology/VirtualGroupAssignmentAlgorithm.java:
##########
@@ -34,5 +34,5 @@ public interface VirtualGroupAssignmentAlgorithm {
    * @return the assignment as mapping from virtual group ID to instanceIds
    */
   Map<String, Set<String>> computeAssignment(int numGroups, String 
virtualGroupName,
-      Map<String, Set<String>> zoneMapping);
+      Map<String, Set<String>> zoneMapping, Map<String, Set<String>> 
virtualZoneMapping);

Review Comment:
   Let's add a new signature with default implementation:
   Ex:
   ```
   default Map<String, Set<String>> computeAssignment(int numGroups, String 
virtualGroupName,
         Map<String, Set<String>> zoneMapping, Map<String, Set<String>> 
virtualZoneMapping) {
         omputeAssignment(int numGroups, String virtualGroupName,
         Map<String, Set<String>> zoneMapping);
   }
   ```



-- 
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-unsubscr...@helix.apache.org

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


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

Reply via email to