IGNITE-26580 Address review comments

Author: sergey-chugunov-1985

modules/core/src/main/java/org/apache/ignite/cache/affinity/rendezvous/MdcAffinityBackupFilter.java

@@ -17,10 +17,9 @@
 
 package org.apache.ignite.cache.affinity.rendezvous;
 
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.internal.util.typedef.internal.S;
 import org.apache.ignite.lang.IgniteBiPredicate;
 
 /**
@@ -77,16 +76,34 @@ public class MdcAffinityBackupFilter implements IgniteBiPredicate<ClusterNode, L
     /** */
     private final int partCopiesPerDc;
 
-    /** Map is used to optimize the time it takes to perform a partition assignment procedure. */
-    private final Map<String, Integer> partsDistrMap;
-
     /**
      * @param dcsNum Number of data centers.
      * @param backups Number of backups.
      */
     public MdcAffinityBackupFilter(int dcsNum, int backups) {
-        partsDistrMap = new HashMap<>(dcsNum + 1);
-        partCopiesPerDc = (backups + 1) / dcsNum;
+        if (dcsNum < 2) {
+            throw new IllegalArgumentException("MdcAffinityBackupFilter cannot be used in an environment with only one datacenter. " +
+                "Number of datacenters must be at least 2.");
+        }
+
+        int numCopies = backups + 1;
+
+        partCopiesPerDc = numCopies / dcsNum;
+        int remainder = numCopies % dcsNum;
+
+        if (remainder != 0) {
+            String suggestion = "recommended ";
+            if (numCopies - remainder <= 0)
+                suggestion += "value is " + (backups + (dcsNum - remainder));
+            else
+                suggestion += "values are " + (backups - remainder) + " and " + (backups + (dcsNum - remainder));
+            
+            throw new IllegalArgumentException("Number of copies is not completely divisible by number of datacenters, " +
+                "copies cannot be distributed evenly across DCs. " +
+                "Please adjust the number of backups, " + suggestion);
+        }
+
+
     }
 
     /**
@@ -98,33 +115,23 @@ public MdcAffinityBackupFilter(int dcsNum, int backups) {
      *                           The primary is first.
      */
     @Override public boolean apply(ClusterNode candidate, List<ClusterNode> previouslySelected) {
-        if (previouslySelected.size() == 1) { //list contains only primary node, thus we started new assignment round.
-            partsDistrMap.replaceAll((e, v) -> -1);
-
-            partsDistrMap.put(previouslySelected.get(0).dataCenterId(), 1);
-        }
-
-        boolean res = false;
         String candidateDcId = candidate.dataCenterId();
+        int candDcCopiesAssigned = 0;
 
-        if (candidateDcId == null)
-            throw new IllegalStateException("Data center ID is not specified for the node: " + candidate);
+        for (int i = 0; i < previouslySelected.size(); i++) {
+            String prevDcId = previouslySelected.get(i).dataCenterId();
 
-        Integer candDcPartsCopies = partsDistrMap.get(candidateDcId);
+            if (prevDcId == null)
+                return false;
 
-        if (candDcPartsCopies == null || candDcPartsCopies == -1) {
-            partsDistrMap.put(candidateDcId, 1);
-
-            res = true;
+            candDcCopiesAssigned += prevDcId.equals(candidateDcId) ? 1 : 0;
         }
-        else {
-            if (candDcPartsCopies < partCopiesPerDc) {
-                partsDistrMap.put(candidateDcId, candDcPartsCopies + 1);
 
-                res = true;
-            }
-        }
+        return candDcCopiesAssigned < partCopiesPerDc;
+    }
 
-        return res;
+    /** {@inheritDoc} */
+    @Override public String toString() {
+        return S.toString(MdcAffinityBackupFilter.class, this);
     }
 }

modules/core/src/test/java/org/apache/ignite/cache/affinity/rendezvous/MdcAffinityBackupFilterSelfTest.java

@@ -23,7 +23,6 @@
 import java.util.Map;
 import java.util.stream.Collectors;
 import org.apache.ignite.IgniteCache;
-import org.apache.ignite.IgniteCheckedException;
 import org.apache.ignite.IgniteSystemProperties;
 import org.apache.ignite.cache.affinity.Affinity;
 import org.apache.ignite.cluster.ClusterNode;
@@ -38,7 +37,7 @@
  */
 public class MdcAffinityBackupFilterSelfTest extends GridCommonAbstractTest {
     /** */
-    private static final int PARTS_CNT = 1024;
+    private static final int PARTS_CNT = 8;
 
     /** */
     private int backups;
@@ -59,18 +58,40 @@ public class MdcAffinityBackupFilterSelfTest extends GridCommonAbstractTest {
         return cfg;
     }
 
-    /** {@inheritDoc} */
-    @Override protected IgniteConfiguration optimize(IgniteConfiguration cfg) throws IgniteCheckedException {
-        return super.optimize(cfg).setIncludeProperties((String[])null);
-    }
-
     /** {@inheritDoc} */
     @Override protected void afterTest() throws Exception {
         super.afterTest();
 
         stopAllGrids();
     }
 
+    /**
+     * Verifies that {@link MdcAffinityBackupFilter} prohibits single data center deployment.
+     */
+    @Test
+    public void testSingleDcDeploymentIsProhibited() {
+        dcIds = new String[] {"DC_0"};
+        System.setProperty(IgniteSystemProperties.IGNITE_DATA_CENTER_ID, dcIds[0]);
+
+        backups = 1;
+        verifyGridFailsToStartWithMessage("Number of datacenters must be at least 2.");
+    }
+
+    /**
+     * Verifies that {@link MdcAffinityBackupFilter} enforces even number of partition copies per datacenter.
+     */
+    @Test
+    public void testEvenNumberOfPartitionCopiesPerDcIsEnforced() {
+        dcIds = new String[] {"DC_0", "DC_1", "DC_2"};
+        System.setProperty(IgniteSystemProperties.IGNITE_DATA_CENTER_ID, dcIds[0]);
+
+        backups = 1;
+        verifyGridFailsToStartWithMessage("recommended value is 2");
+
+        backups = 7;
+        verifyGridFailsToStartWithMessage("recommended values are 5 and 8");
+    }
+
     /**
      * Verifies that partition copies are assigned evenly across a cluster in two data centers.
      * <p/>
@@ -119,6 +140,19 @@ public void test3DcDistribution() throws Exception {
         verifyDistributionProperties(srv, dcIds, nodesPerDc, backups);
     }
 
+    /** */
+    private void verifyGridFailsToStartWithMessage(String msg) {
+        try {
+            startGrid(0);
+        } catch (IllegalArgumentException argEx) {
+            String errMsg = argEx.getMessage();
+
+            assertTrue(errMsg.contains(msg));
+        } catch (Exception e) {
+            fail("Unexpected exception was thrown: " + e);
+        }
+    }
+
     /** Starts specified number of nodes in each DC. */
     private IgniteEx startClusterAcrossDataCenters(String[] dcIds, int nodesPerDc) throws Exception {
         int nodeIdx = 0;