Drop the get and contains in favor of computeIfPresent

Author: joegallo

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java

@@ -151,24 +151,11 @@ public void onClose(IndexReader.CacheKey indexKey) {
     private void onCacheEviction(RemovalNotification<BitsetCacheKey, BitSet> notification) {
         final BitsetCacheKey cacheKey = notification.getKey();
         final IndexReader.CacheKey indexKey = cacheKey.indexKey;
-        if (keysByIndex.getOrDefault(indexKey, Set.of()).contains(cacheKey) == false) {
-            // If the cacheKey isn't in the lookup map, then there's nothing to synchronize
-            return;
-        }
-
-        // it's possible for the key to be back in the cache if it was immediately repopulated after it was evicted, so check
-        if (bitsetCache.get(cacheKey) == null) {
-            // key is no longer in the cache, make sure it is no longer in the lookup map either.
-            keysByIndex.compute(indexKey, (ignored, keys) -> {
-                if (keys != null) {
-                    keys.remove(cacheKey);
-                    if (keys.isEmpty()) {
-                        keys = null;
-                    }
-                }
-                return keys;
-            });
-        }
+        // key is no longer in the cache, make sure it is no longer in the lookup map either.
+        keysByIndex.computeIfPresent(indexKey, (ignored, keys) -> {
+            keys.remove(cacheKey);
+            return keys.isEmpty() ? null : keys;
+        });
     }
 
     @Override

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java

@@ -547,20 +547,17 @@ public void testHitsMissesAndEvictionsStats() throws Exception {
             final Query query2 = QueryBuilders.termQuery("field-2", "value-2").toQuery(searchExecutionContext);
             final BitSet bitSet2 = cache.getBitSet(query2, leafContext);
             assertThat(bitSet2, notNullValue());
-            // surprisingly, the eviction callback can call `get` on the cache (asynchronously) which causes another miss (or hit)
-            // so this assertion is about the current state of the code, rather than the expected or desired state.
-            // see https://github.com/elastic/elasticsearch/issues/132842
-            expectedStats.put("misses", 3L);
+            expectedStats.put("misses", 2L);
             expectedStats.put("evictions", 1L);
             // underlying Cache class tracks hits/misses, but timing is in DLS cache, which is why we have `2L` here,
             // because DLS cache is only hit once
             expectedStats.put("misses_time_in_millis", 2L);
-            assertBusy(() -> { assertThat(cache.usageStats(), equalTo(expectedStats)); }, 200, TimeUnit.MILLISECONDS);
+            assertThat(cache.usageStats(), equalTo(expectedStats));
         });
 
         final Map<String, Object> finalStats = emptyStatsSupplier.get();
         finalStats.put("hits", 1L);
-        finalStats.put("misses", 3L);
+        finalStats.put("misses", 2L);
         finalStats.put("evictions", 2L);
         finalStats.put("hits_time_in_millis", 1L);
         finalStats.put("misses_time_in_millis", 2L);