Exclusion Hunter I: CacheLoaderNull (#5611)

palantir · Aug 27, 2021 · c31a411 · c31a411
1 parent 964c1a2
commit c31a411
Show file tree

Hide file tree

Showing 13 changed files with 60 additions and 55 deletions.
diff --git a/...-client/src/main/java/com/palantir/atlasdb/transaction/impl/ConflictDetectionManager.java b/...-client/src/main/java/com/palantir/atlasdb/transaction/impl/ConflictDetectionManager.java
@@ -21,10 +21,11 @@
 import com.palantir.atlasdb.keyvalue.api.TableReference;
 import com.palantir.atlasdb.transaction.api.ConflictHandler;
 import java.util.Map;
+import java.util.Optional;
 import javax.annotation.Nullable;
 
 public class ConflictDetectionManager {
-    private final LoadingCache<TableReference, ConflictHandler> cache;
+    private final LoadingCache<TableReference, Optional<ConflictHandler>> cache;
 
     /**
      *  This class does not make the mistake of attempting cache invalidation,
@@ -38,20 +39,20 @@ public class ConflictDetectionManager {
      *
      *  (This has always been the behavior of this class; I'm simply calling it out)
      */
-    public ConflictDetectionManager(CacheLoader<TableReference, ConflictHandler> loader) {
+    public ConflictDetectionManager(CacheLoader<TableReference, Optional<ConflictHandler>> loader) {
         this.cache = CacheBuilder.newBuilder().maximumSize(100_000).build(loader);
     }
 
-    public void warmCacheWith(Map<TableReference, ConflictHandler> preload) {
+    public void warmCacheWith(Map<TableReference, Optional<ConflictHandler>> preload) {
         cache.putAll(preload);
     }
 
-    public Map<TableReference, ConflictHandler> getCachedValues() {
+    public Map<TableReference, Optional<ConflictHandler>> getCachedValues() {
         return cache.asMap();
     }
 
     @Nullable
-    public ConflictHandler get(TableReference tableReference) {
+    public Optional<ConflictHandler> get(TableReference tableReference) {
         return cache.getUnchecked(tableReference);
     }
 }
diff --git a/...client/src/main/java/com/palantir/atlasdb/transaction/impl/ConflictDetectionManagers.java b/...client/src/main/java/com/palantir/atlasdb/transaction/impl/ConflictDetectionManagers.java
@@ -21,6 +21,7 @@
 import com.palantir.atlasdb.keyvalue.api.TableReference;
 import com.palantir.atlasdb.table.description.TableMetadata;
 import com.palantir.atlasdb.transaction.api.ConflictHandler;
+import java.util.Optional;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -30,10 +31,10 @@ public final class ConflictDetectionManagers {
     private ConflictDetectionManagers() {}
 
     public static ConflictDetectionManager createWithNoConflictDetection() {
-        return new ConflictDetectionManager(new CacheLoader<TableReference, ConflictHandler>() {
+        return new ConflictDetectionManager(new CacheLoader<>() {
             @Override
-            public ConflictHandler load(TableReference tableReference) throws Exception {
-                return ConflictHandler.IGNORE_ALL;
+            public Optional<ConflictHandler> load(TableReference tableReference) throws Exception {
+                return Optional.of(ConflictHandler.IGNORE_ALL);
             }
         });
     }
@@ -63,21 +64,18 @@ public static ConflictDetectionManager create(KeyValueService kvs) {
     }
 
     private static ConflictDetectionManager create(KeyValueService kvs, boolean warmCache) {
-        ConflictDetectionManager conflictDetectionManager =
-                new ConflictDetectionManager(new CacheLoader<TableReference, ConflictHandler>() {
-                    @Override
-                    public ConflictHandler load(TableReference tableReference) throws Exception {
-                        byte[] metadata = kvs.getMetadataForTable(tableReference);
-                        if (metadata == null) {
-                            log.error(
-                                    "Tried to make a transaction over a table that has no metadata: {}.",
-                                    tableReference);
-                            return null;
-                        } else {
-                            return getConflictHandlerFromMetadata(metadata);
-                        }
-                    }
-                });
+        ConflictDetectionManager conflictDetectionManager = new ConflictDetectionManager(new CacheLoader<>() {
+            @Override
+            public Optional<ConflictHandler> load(TableReference tableReference) throws Exception {
+                byte[] metadata = kvs.getMetadataForTable(tableReference);
+                if (metadata == null) {
+                    log.error("Tried to make a transaction over a table that has no metadata: {}.", tableReference);
+                    return Optional.empty();
+                } else {
+                    return Optional.of(getConflictHandlerFromMetadata(metadata));
+                }
+            }
+        });
         if (warmCache) {
             // kick off an async thread that attempts to fully warm this cache
             // if it fails (e.g. probably this user has way too many tables), that's okay,
@@ -91,9 +89,9 @@ public ConflictHandler load(TableReference tableReference) throws Exception {
                                                     log.debug("Metadata was null for a table. likely because the table"
                                                             + " is currently  being created. Skipping warming"
                                                             + " cache for the table.");
-                                                    return null;
+                                                    return Optional.empty();
                                                 } else {
-                                                    return getConflictHandlerFromMetadata(metadata);
+                                                    return Optional.of(getConflictHandlerFromMetadata(metadata));
                                                 }
                                             }));
                                 } catch (Throwable t) {

diff --git a/.../main/java/com/palantir/atlasdb/transaction/impl/TransactionConflictDetectionManager.java b/.../main/java/com/palantir/atlasdb/transaction/impl/TransactionConflictDetectionManager.java
@@ -37,9 +37,10 @@ public TransactionConflictDetectionManager(ConflictDetectionManager delegate) {
 
     public void disableReadWriteConflict(TableReference tableRef) {
         conflictHandlers.compute(tableRef, (tableReference, nullableCurrentValue) -> {
-            ConflictHandler conflictHandler = delegate.get(tableReference);
-            Preconditions.checkNotNull(conflictHandler, "Conflict handler cannot be null when overwriting");
-            Optional<ConflictHandler> newValue = Optional.of(getDisabledReadWriteConflictHandler(conflictHandler));
+            Optional<ConflictHandler> conflictHandler = delegate.get(tableReference);
+            Preconditions.checkState(conflictHandler.isPresent(), "Conflict handler cannot be null when overwriting");
+            Optional<ConflictHandler> newValue =
+                    Optional.of(getDisabledReadWriteConflictHandler(conflictHandler.get()));
 
             Preconditions.checkState(
                     nullableCurrentValue == null || nullableCurrentValue.equals(newValue),
@@ -68,8 +69,6 @@ private ConflictHandler getDisabledReadWriteConflictHandler(ConflictHandler conf
 
     @Nullable
     public ConflictHandler get(TableReference tableReference) {
-        return conflictHandlers
-                .computeIfAbsent(tableReference, tableRef -> Optional.ofNullable(delegate.get(tableRef)))
-                .orElse(null);
+        return conflictHandlers.computeIfAbsent(tableReference, delegate::get).orElse(null);
     }
 }
diff --git a/...t/java/com/palantir/atlasdb/transaction/impl/TransactionConflictDetectionManagerTest.java b/...t/java/com/palantir/atlasdb/transaction/impl/TransactionConflictDetectionManagerTest.java
@@ -26,7 +26,7 @@
 import com.palantir.atlasdb.keyvalue.api.TableReference;
 import com.palantir.atlasdb.transaction.api.ConflictHandler;
 import com.palantir.logsafe.exceptions.SafeIllegalStateException;
-import com.palantir.logsafe.exceptions.SafeNullPointerException;
+import java.util.Optional;
 import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
@@ -41,7 +41,7 @@ public final class TransactionConflictDetectionManagerTest {
             TableReference.create(Namespace.EMPTY_NAMESPACE, "test_table");
 
     @Mock
-    private CacheLoader<TableReference, ConflictHandler> delegate;
+    private CacheLoader<TableReference, Optional<ConflictHandler>> delegate;
 
     private TransactionConflictDetectionManager conflictDetectionManager;
 
@@ -50,7 +50,7 @@ public void before() {
         conflictDetectionManager = new TransactionConflictDetectionManager(new ConflictDetectionManager(delegate) {
             @Override
             @Nullable
-            public ConflictHandler get(TableReference tableReference) {
+            public Optional<ConflictHandler> get(TableReference tableReference) {
                 try {
                     return delegate.load(tableReference);
                 } catch (Exception e) {
@@ -63,7 +63,7 @@ public ConflictHandler get(TableReference tableReference) {
     @Test
     public void testDisableReadWriteConflict_throwsIfDelegateValueIsNull() {
         assertThatLoggableExceptionThrownBy(() -> whenDisableReadWriteConflict(null))
-                .isExactlyInstanceOf(SafeNullPointerException.class)
+                .isExactlyInstanceOf(SafeIllegalStateException.class)
                 .hasLogMessage("Conflict handler cannot be null when overwriting");
     }
 
@@ -75,7 +75,7 @@ public void testDisableReadWriteConflict_multipleCalls() throws Exception {
 
     @Test
     public void testDisableReadWriteConflict_throwsIfCalledAfterTableWasUsed() throws Exception {
-        when(delegate.load(TABLE_REFERENCE)).thenReturn(ConflictHandler.SERIALIZABLE);
+        when(delegate.load(TABLE_REFERENCE)).thenReturn(Optional.of(ConflictHandler.SERIALIZABLE));
         conflictDetectionManager.get(TABLE_REFERENCE);
         assertThatLoggableExceptionThrownBy(() -> conflictDetectionManager.disableReadWriteConflict(TABLE_REFERENCE))
                 .isExactlyInstanceOf(SafeIllegalStateException.class)
@@ -130,7 +130,7 @@ private void testDisableReadWriteConflictThrowsUnsupported(ConflictHandler initi
     }
 
     private void whenDisableReadWriteConflict(ConflictHandler initial) throws Exception {
-        when(delegate.load(TABLE_REFERENCE)).thenReturn(initial);
+        when(delegate.load(TABLE_REFERENCE)).thenReturn(Optional.ofNullable(initial));
         conflictDetectionManager.disableReadWriteConflict(TABLE_REFERENCE);
     }
 }
diff --git a/.../main/java/com/palantir/atlasdb/transaction/impl/AbstractSerializableTransactionTest.java b/.../main/java/com/palantir/atlasdb/transaction/impl/AbstractSerializableTransactionTest.java
@@ -129,11 +129,11 @@ protected Transaction startTransaction() {
     }
 
     private Transaction startTransactionWithOptions(TransactionOptions options) {
-        ImmutableMap<TableReference, ConflictHandler> tablesToWriteWrite = ImmutableMap.of(
+        ImmutableMap<TableReference, Optional<ConflictHandler>> tablesToWriteWrite = ImmutableMap.of(
                 TEST_TABLE,
-                ConflictHandler.SERIALIZABLE,
+                Optional.of(ConflictHandler.SERIALIZABLE),
                 TransactionConstants.TRANSACTION_TABLE,
-                ConflictHandler.IGNORE_ALL);
+                Optional.of(ConflictHandler.IGNORE_ALL));
         return new SerializableTransaction(
                 MetricsManagers.createForTests(),
                 keyValueService,

diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/CommitLockTest.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/CommitLockTest.java
@@ -154,8 +154,11 @@ private void commitWriteWith(PreCommitCondition preCommitCondition, ConflictHand
     }
 
     private Transaction startTransaction(PreCommitCondition preCommitCondition, ConflictHandler conflictHandler) {
-        ImmutableMap<TableReference, ConflictHandler> tablesToWriteWrite = ImmutableMap.of(
-                TEST_TABLE, conflictHandler, TransactionConstants.TRANSACTION_TABLE, ConflictHandler.IGNORE_ALL);
+        ImmutableMap<TableReference, Optional<ConflictHandler>> tablesToWriteWrite = ImmutableMap.of(
+                TEST_TABLE,
+                Optional.of(conflictHandler),
+                TransactionConstants.TRANSACTION_TABLE,
+                Optional.of(ConflictHandler.IGNORE_ALL));
         return new SerializableTransaction(
                 MetricsManagers.createForTests(),
                 keyValueService,

diff --git a/...ed/src/main/java/com/palantir/atlasdb/transaction/impl/TestConflictDetectionManagers.java b/...ed/src/main/java/com/palantir/atlasdb/transaction/impl/TestConflictDetectionManagers.java
@@ -20,16 +20,18 @@
 import com.palantir.atlasdb.keyvalue.api.TableReference;
 import com.palantir.atlasdb.transaction.api.ConflictHandler;
 import java.util.Map;
+import java.util.Optional;
 
 public final class TestConflictDetectionManagers {
     private TestConflictDetectionManagers() {}
 
     @VisibleForTesting
-    static ConflictDetectionManager createWithStaticConflictDetection(Map<TableReference, ConflictHandler> staticMap) {
-        return new ConflictDetectionManager(new CacheLoader<TableReference, ConflictHandler>() {
+    static ConflictDetectionManager createWithStaticConflictDetection(
+            Map<TableReference, Optional<ConflictHandler>> staticMap) {
+        return new ConflictDetectionManager(new CacheLoader<>() {
             @Override
-            public ConflictHandler load(TableReference tableReference) throws Exception {
-                return staticMap.getOrDefault(tableReference, ConflictHandler.RETRY_ON_WRITE_WRITE);
+            public Optional<ConflictHandler> load(TableReference tableReference) throws Exception {
+                return staticMap.getOrDefault(tableReference, Optional.of(ConflictHandler.RETRY_ON_WRITE_WRITE));
             }
         });
     }

diff --git a/...ts-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManager.java b/...ts-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManager.java
@@ -19,13 +19,14 @@
 import com.palantir.atlasdb.transaction.api.ConflictHandler;
 import com.palantir.atlasdb.transaction.api.Transaction;
 import com.palantir.atlasdb.transaction.api.TransactionManager;
+import java.util.Optional;
 
 public interface TestTransactionManager extends TransactionManager {
     Transaction commitAndStartNewTransaction(Transaction txn);
 
     Transaction createNewTransaction();
 
-    void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler);
+    void overrideConflictHandlerForTable(TableReference table, Optional<ConflictHandler> conflictHandler);
 
     void setUnreadableTimestamp(long timestamp);
 }
diff --git a/...hared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManagerImpl.java b/...hared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManagerImpl.java
@@ -52,7 +52,7 @@ public class TestTransactionManagerImpl extends SerializableTransactionManager i
     static final TransactionConfig TRANSACTION_CONFIG =
             ImmutableTransactionConfig.builder().build();
 
-    private final Map<TableReference, ConflictHandler> conflictHandlerOverrides = new HashMap<>();
+    private final Map<TableReference, Optional<ConflictHandler>> conflictHandlerOverrides = new HashMap<>();
     private final WrapperWithTracker<Transaction> transactionWrapper;
     private final WrapperWithTracker<KeyValueService> keyValueServiceWrapper;
     private Optional<Long> unreadableTs = Optional.empty();
@@ -258,7 +258,7 @@ ConflictDetectionManager getConflictDetectionManager() {
     }
 
     @Override
-    public void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler) {
+    public void overrideConflictHandlerForTable(TableReference table, Optional<ConflictHandler> conflictHandler) {
         conflictHandlerOverrides.put(table, conflictHandler);
     }
 
@@ -272,8 +272,8 @@ public void setUnreadableTimestamp(long timestamp) {
         unreadableTs = Optional.of(timestamp);
     }
 
-    private Map<TableReference, ConflictHandler> getConflictHandlerWithOverrides() {
-        Map<TableReference, ConflictHandler> conflictHandlersWithOverrides = new HashMap<>();
+    private Map<TableReference, Optional<ConflictHandler>> getConflictHandlerWithOverrides() {
+        Map<TableReference, Optional<ConflictHandler>> conflictHandlersWithOverrides = new HashMap<>();
         conflictHandlersWithOverrides.putAll(conflictDetectionManager.getCachedValues());
         conflictHandlersWithOverrides.putAll(conflictHandlerOverrides);
         return conflictHandlersWithOverrides;

diff --git a/...d/src/main/java/com/palantir/atlasdb/transaction/impl/WrappingTestTransactionManager.java b/...d/src/main/java/com/palantir/atlasdb/transaction/impl/WrappingTestTransactionManager.java
@@ -18,6 +18,7 @@
 import com.palantir.atlasdb.keyvalue.api.TableReference;
 import com.palantir.atlasdb.transaction.api.ConflictHandler;
 import com.palantir.atlasdb.transaction.api.Transaction;
+import java.util.Optional;
 
 public abstract class WrappingTestTransactionManager extends WrappingTransactionManager
         implements TestTransactionManager {
@@ -39,7 +40,7 @@ public Transaction createNewTransaction() {
     }
 
     @Override
-    public void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler) {
+    public void overrideConflictHandlerForTable(TableReference table, Optional<ConflictHandler> conflictHandler) {
         delegate.overrideConflictHandlerForTable(table, conflictHandler);
     }
 

diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/AtlasDbTestCase.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/AtlasDbTestCase.java
@@ -48,6 +48,7 @@
 import com.palantir.lock.LockService;
 import com.palantir.lock.impl.LockServiceImpl;
 import com.palantir.timestamp.InMemoryTimestampService;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import org.junit.After;
@@ -137,7 +138,7 @@ public void tearDown() throws Exception {
     }
 
     protected void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler) {
-        txManager.overrideConflictHandlerForTable(table, conflictHandler);
+        txManager.overrideConflictHandlerForTable(table, Optional.of(conflictHandler));
     }
 
     protected void setConstraintCheckingMode(AtlasDbConstraintCheckingMode mode) {

diff --git a/...s-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/...s-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java
@@ -2119,7 +2119,7 @@ private Transaction getSnapshotTransactionWith(
                         NoOpCleaner.INSTANCE,
                         startTs,
                         TestConflictDetectionManagers.createWithStaticConflictDetection(
-                                ImmutableMap.of(TABLE, ConflictHandler.RETRY_ON_WRITE_WRITE)),
+                                ImmutableMap.of(TABLE, Optional.of(ConflictHandler.RETRY_ON_WRITE_WRITE))),
                         SweepStrategyManagers.createDefault(keyValueService),
                         lockImmutableTimestampResponse.getImmutableTimestamp(),
                         Optional.of(lockImmutableTimestampResponse.getLock()),

diff --git a/build.gradle b/build.gradle
@@ -103,7 +103,6 @@ allprojects {
         options.compilerArgs += ['-Werror']
         // temporarily relax constraints until we can fix all violations
         options.errorprone.disable 'ArrayAsKeyOfSetOrMap',
-                'CacheLoaderNull',
                 'CloseableProvides',
                 'DangerousCompletableFutureUsage',
                 'DnsLookup',