update FSSDK-11899: implement lock striping for improved concurrency in DefaultCmabService

Author: FarhanAnjum-opti

core-api/src/main/java/com/optimizely/ab/cmab/service/DefaultCmabService.java

@@ -20,6 +20,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.slf4j.Logger;
 
@@ -33,52 +34,67 @@
 import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption;
 
 public class DefaultCmabService implements CmabService {
+    private static final int NUM_LOCK_STRIPES = 1000;
 
     private final DefaultLRUCache<CmabCacheValue> cmabCache;
     private final CmabClient cmabClient;
     private final Logger logger;
+    private final ReentrantLock[] locks;
 
     public DefaultCmabService(CmabServiceOptions options) {
         this.cmabCache = options.getCmabCache();
         this.cmabClient = options.getCmabClient();
         this.logger = options.getLogger();
+
+        this.locks = new ReentrantLock[NUM_LOCK_STRIPES];
+        for (int i = 0; i < NUM_LOCK_STRIPES; i++) {
+            this.locks[i] = new ReentrantLock();
+        }
     }
 
     @Override
     public CmabDecision getDecision(ProjectConfig projectConfig, OptimizelyUserContext userContext, String ruleId, List<OptimizelyDecideOption> options) {
         options = options == null ? Collections.emptyList() : options;
         String userId = userContext.getUserId();
-        Map<String, Object> filteredAttributes = filterAttributes(projectConfig, userContext, ruleId);
 
-        if (options.contains(OptimizelyDecideOption.IGNORE_CMAB_CACHE)) {
-            return fetchDecision(ruleId, userId, filteredAttributes);
-        }
+        int lockIndex = getLockIndex(userId, ruleId);
+        ReentrantLock lock = locks[lockIndex];
+        lock.lock();
+        try {
+            Map<String, Object> filteredAttributes = filterAttributes(projectConfig, userContext, ruleId);
 
-        if (options.contains(OptimizelyDecideOption.RESET_CMAB_CACHE)) {
-            cmabCache.reset();
-        }
+            if (options.contains(OptimizelyDecideOption.IGNORE_CMAB_CACHE)) {
+                return fetchDecision(ruleId, userId, filteredAttributes);
+            }
 
-        String cacheKey = getCacheKey(userContext.getUserId(), ruleId);
-        if (options.contains(OptimizelyDecideOption.INVALIDATE_USER_CMAB_CACHE)) {
-            cmabCache.remove(cacheKey);
-        }
+            if (options.contains(OptimizelyDecideOption.RESET_CMAB_CACHE)) {
+                cmabCache.reset();
+            }
 
-        CmabCacheValue cachedValue = cmabCache.lookup(cacheKey);
+            String cacheKey = getCacheKey(userContext.getUserId(), ruleId);
+            if (options.contains(OptimizelyDecideOption.INVALIDATE_USER_CMAB_CACHE)) {
+                cmabCache.remove(cacheKey);
+            }
 
-        String attributesHash = hashAttributes(filteredAttributes);
+            CmabCacheValue cachedValue = cmabCache.lookup(cacheKey);
 
-        if (cachedValue != null) {
-            if (cachedValue.getAttributesHash().equals(attributesHash)) {
-                return new CmabDecision(cachedValue.getVariationId(), cachedValue.getCmabUuid());
-            } else {
-                cmabCache.remove(cacheKey);
+            String attributesHash = hashAttributes(filteredAttributes);
+
+            if (cachedValue != null) {
+                if (cachedValue.getAttributesHash().equals(attributesHash)) {
+                    return new CmabDecision(cachedValue.getVariationId(), cachedValue.getCmabUuid());
+                } else {
+                    cmabCache.remove(cacheKey);
+                }
             }
-        }
 
-        CmabDecision cmabDecision = fetchDecision(ruleId, userId, filteredAttributes);
-        cmabCache.save(cacheKey, new CmabCacheValue(attributesHash, cmabDecision.getVariationId(), cmabDecision.getCmabUUID()));
+            CmabDecision cmabDecision = fetchDecision(ruleId, userId, filteredAttributes);
+            cmabCache.save(cacheKey, new CmabCacheValue(attributesHash, cmabDecision.getVariationId(), cmabDecision.getCmabUUID()));
 
-        return cmabDecision;
+            return cmabDecision;
+        } finally {
+            lock.unlock();
+        }
     }
 
     private CmabDecision fetchDecision(String ruleId, String userId, Map<String, Object> attributes) {
@@ -182,4 +198,11 @@ private String hashAttributes(Map<String, Object> attributes) {
         // Convert to hex string to match your existing pattern
         return Integer.toHexString(hash);
     }
+
+    private int getLockIndex(String userId, String ruleId) {
+        // Create a hash of userId + ruleId for consistent lock selection
+        String combined = userId + ruleId;
+        int hash = MurmurHash3.murmurhash3_x86_32(combined, 0, combined.length(), 0);
+        return Math.abs(hash) % NUM_LOCK_STRIPES;
+    }
 }

core-api/src/test/java/com/optimizely/ab/cmab/DefaultCmabServiceTest.java

@@ -15,15 +15,20 @@
  */
 package com.optimizely.ab.cmab;
 
+import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
@@ -378,4 +383,62 @@ public void testAttributeOrderDoesNotMatterForCaching() {
         assertNotNull(decision.getCmabUUID());
         verify(mockCmabCache).save(eq(cacheKey), any(CmabCacheValue.class));
     }
+
+    @Test
+    public void testLockStripingDistribution() {
+        // Test different combinations to ensure they get different lock indices
+        String[][] testCases = {
+            {"user1", "rule1"},
+            {"user2", "rule1"},
+            {"user1", "rule2"},
+            {"user3", "rule3"},
+            {"user4", "rule4"}
+        };
+
+        Set<Integer> lockIndices = new HashSet<>();
+        for (String[] testCase : testCases) {
+            String userId = testCase[0];
+            String ruleId = testCase[1];
+
+            // Use reflection to access the private getLockIndex method
+            try {
+                Method getLockIndexMethod = DefaultCmabService.class.getDeclaredMethod("getLockIndex", String.class, String.class);
+                getLockIndexMethod.setAccessible(true);
+
+                int index = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
+
+                // Verify index is within expected range
+                assertTrue("Lock index should be non-negative", index >= 0);
+                assertTrue("Lock index should be less than NUM_LOCK_STRIPES", index < 1000);
+
+                lockIndices.add(index);
+            } catch (Exception e) {
+                fail("Failed to invoke getLockIndex method: " + e.getMessage());
+            }
+        }
+
+        assertTrue("Different user/rule combinations should generally use different locks", lockIndices.size() > 1);
+    }
+
+    @Test
+    public void testSameUserRuleCombinationUsesConsistentLock() {
+        String userId = "test_user";
+        String ruleId = "test_rule";
+
+        try {
+            Method getLockIndexMethod = DefaultCmabService.class.getDeclaredMethod("getLockIndex", String.class, String.class);
+            getLockIndexMethod.setAccessible(true);
+
+            // Get lock index multiple times
+            int index1 = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
+            int index2 = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
+            int index3 = (Integer) getLockIndexMethod.invoke(cmabService, userId, ruleId);
+
+            // All should be the same
+            assertEquals("Same user/rule should always use same lock", index1, index2);
+            assertEquals("Same user/rule should always use same lock", index2, index3);
+        } catch (Exception e) {
+            fail("Failed to invoke getLockIndex method: " + e.getMessage());
+        }
+    }
 }