From bce5cf29c6035f622c6b916320d45986de4fa6d5 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 30 Sep 2019 19:19:15 +0100 Subject: [PATCH 01/16] Prerequisite --- .../keyvalue/cassandra/async/ThrowingCqlClientImpl.java | 5 +++++ .../com/palantir/atlasdb/logging/KvsProfilingLogger.java | 3 +-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java index 1070af0c2a9..9144a30ea0c 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java @@ -34,4 +34,9 @@ public void close() { public ListenableFuture executeQuery(CqlQuerySpec querySpec) { throw new UnsupportedOperationException("Not configured to use CQL client, check your KVS config file."); } + + @Override + public ListenableFuture execute(Executable executable) { + throw new UnsupportedOperationException("Not configured to use CQL client, check your KVS config file."); + } } diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java index 0c1a0be73ba..9a13be480e1 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java @@ -23,7 +23,6 @@ import java.util.function.Predicate; import java.util.function.Supplier; -import org.checkerframework.checker.nullness.compatqual.NullableDecl; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -158,7 +157,7 @@ public static ListenableFuture maybeLogAsync( slowLogPredicate); Futures.addCallback(future, new FutureCallback() { @Override - public void onSuccess(@NullableDecl T result) { + public void onSuccess(T result) { monitor.registerResult(result); monitor.log(); } From 4fb08d41dc8b32d4bfe5472f77206ad0abc7dbca Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Fri, 11 Oct 2019 13:50:40 +0100 Subject: [PATCH 02/16] Initial getAsync on transaction implementation --- .../atlasdb/transaction/api/Transaction.java | 4 + .../impl/ForwardingTransaction.java | 6 + .../impl/NoDuplicateWritesTransaction.java | 2 +- .../transaction/impl/ReadTransaction.java | 7 + .../transaction/impl/SnapshotTransaction.java | 126 +++++++++++++----- 5 files changed, 108 insertions(+), 37 deletions(-) diff --git a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java index 4842e3a2400..8d1dea3cc59 100644 --- a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java +++ b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java @@ -22,6 +22,7 @@ import java.util.function.BiFunction; import java.util.stream.Stream; +import com.google.common.util.concurrent.ListenableFuture; import com.palantir.atlasdb.keyvalue.api.BatchColumnRangeSelection; import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.ColumnRangeSelection; @@ -238,4 +239,7 @@ enum TransactionType { default void disableReadWriteConflictChecking(TableReference tableRef) { throw new UnsupportedOperationException(); } + + @Idempotent + ListenableFuture> getAsync(TableReference tableRef, Set cells); } diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java index 007aa763f2e..37728336b3d 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ForwardingTransaction.java @@ -24,6 +24,7 @@ import java.util.stream.Stream; import com.google.common.collect.ForwardingObject; +import com.google.common.util.concurrent.ListenableFuture; import com.palantir.atlasdb.keyvalue.api.BatchColumnRangeSelection; import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.ColumnRangeSelection; @@ -175,4 +176,9 @@ public TransactionType getTransactionType() { public void disableReadWriteConflictChecking(TableReference tableRef) { delegate().disableReadWriteConflictChecking(tableRef); } + + @Override + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + return delegate().getAsync(tableRef, cells); + } } diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/NoDuplicateWritesTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/NoDuplicateWritesTransaction.java index c36a83a7ad0..8ee3a54410f 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/NoDuplicateWritesTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/NoDuplicateWritesTransaction.java @@ -50,7 +50,7 @@ public class NoDuplicateWritesTransaction extends ForwardingTransaction { new CacheLoader>() { @Override public Map load(TableReference input) { - return Collections.synchronizedMap(Maps.newHashMap()); + return Collections.synchronizedMap(Maps.newHashMap()); } }); diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java index b462141138e..1d570e92c87 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java @@ -22,6 +22,7 @@ import java.util.function.BiFunction; import java.util.stream.Stream; +import com.google.common.util.concurrent.ListenableFuture; import com.palantir.atlasdb.keyvalue.api.BatchColumnRangeSelection; import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.ColumnRangeSelection; @@ -142,4 +143,10 @@ public void delete(TableReference tableRef, Set keys) { throw new SafeIllegalArgumentException("This is a read only transaction."); } + @Override + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + checkTableName(tableRef); + return delegate().getAsync(tableRef, cells); + } + } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index 4480021dd02..6e0b2e85e31 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -314,6 +314,43 @@ public void disableReadWriteConflictChecking(TableReference tableRef) { conflictDetectionManager.disableReadWriteConflict(tableRef); } + @Override + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + Timer.Context timer = getTimer("get").time(); + checkGetPreconditions(tableRef); + if (Iterables.isEmpty(cells)) { + return Futures.immediateFuture(ImmutableMap.of()); + } + hasReads = true; + + Map result = Maps.newHashMap(); + SortedMap writes = writesByTable.get(tableRef); + if (writes != null) { + for (Cell cell : cells) { + if (writes.containsKey(cell)) { + result.put(cell, writes.get(cell)); + } + } + } + + return Futures.transform( + // We don't need to read any cells that were written locally. + getFromKeyValueServiceAsync(tableRef, Sets.difference(cells, result.keySet())), + keyValueResult -> { + result.putAll(keyValueResult); + + long getMillis = TimeUnit.NANOSECONDS.toMillis(timer.stop()); + if (perfLogger.isDebugEnabled()) { + perfLogger.debug("get({}, {} cells) found {} cells (some possibly deleted), took {} ms", + tableRef, cells.size(), result.size(), getMillis); + } + validatePreCommitRequirementsOnReadIfNecessary(tableRef, getStartTimestamp()); + return removeEmptyColumns(result, tableRef); + }, + MoreExecutors.directExecutor()); + } + + @Override public SortedMap> getRows(TableReference tableRef, Iterable rows, ColumnSelection columnSelection) { @@ -347,7 +384,7 @@ public SortedMap> getRows(TableReference tableRef, Ite } @Override - public Map>> getRowsColumnRange( + public Map>> getRowsColumnRange( TableReference tableRef, Iterable rows, BatchColumnRangeSelection columnRangeSelection) { @@ -356,7 +393,7 @@ public Map>> getRowsColumnRang } @Override - public Iterator> getRowsColumnRange(TableReference tableRef, + public Iterator> getRowsColumnRange(TableReference tableRef, Iterable rows, ColumnRangeSelection columnRangeSelection, int batchHint) { @@ -375,8 +412,8 @@ public Iterator> getRowsColumnRange(TableReference table validatePreCommitRequirementsOnReadIfNecessary(tableRef, getStartTimestamp()); } // else the postFiltered iterator will check for each batch. - Iterator> rawResultsByRow = partitionByRow(rawResults); - Iterator>> postFiltered = Iterators.transform(rawResultsByRow, e -> { + Iterator> rawResultsByRow = partitionByRow(rawResults); + Iterator>> postFiltered = Iterators.transform(rawResultsByRow, e -> { byte[] row = e.getKey(); RowColumnRangeIterator rawIterator = e.getValue(); BatchColumnRangeSelection batchColumnRangeSelection = @@ -388,7 +425,7 @@ public Iterator> getRowsColumnRange(TableReference table } @Override - public Map>> getRowsColumnRangeIterator(TableReference tableRef, + public Map>> getRowsColumnRangeIterator(TableReference tableRef, Iterable rows, BatchColumnRangeSelection columnRangeSelection) { checkGetPreconditions(tableRef); @@ -398,28 +435,28 @@ public Map>> getRowsColumnRangeIterator hasReads = true; Map rawResults = keyValueService.getRowsColumnRange(tableRef, rows, columnRangeSelection, getStartTimestamp()); - Map>> postFilteredResults = + Map>> postFilteredResults = Maps.newHashMapWithExpectedSize(rawResults.size()); for (Entry e : rawResults.entrySet()) { byte[] row = e.getKey(); RowColumnRangeIterator rawIterator = e.getValue(); - Iterator> postFilteredIterator = + Iterator> postFilteredIterator = getPostFilteredColumns(tableRef, columnRangeSelection, row, rawIterator); postFilteredResults.put(row, postFilteredIterator); } return postFilteredResults; } - private Iterator> getPostFilteredColumns( + private Iterator> getPostFilteredColumns( TableReference tableRef, BatchColumnRangeSelection batchColumnRangeSelection, byte[] row, RowColumnRangeIterator rawIterator) { - Iterator> postFilterIterator = + Iterator> postFilterIterator = getRowColumnRangePostFiltered(tableRef, row, batchColumnRangeSelection, rawIterator); SortedMap localWrites = getLocalWritesForColumnRange(tableRef, batchColumnRangeSelection, row); - Iterator> localIterator = localWrites.entrySet().iterator(); - Iterator> mergedIterator = + Iterator> localIterator = localWrites.entrySet().iterator(); + Iterator> mergedIterator = IteratorUtils.mergeIterators(localIterator, postFilterIterator, Ordering.from(UnsignedBytes.lexicographicalComparator()) @@ -429,8 +466,8 @@ private Iterator> getPostFilteredColumns( return filterDeletedValues(mergedIterator, tableRef); } - private Iterator> filterDeletedValues( - Iterator> unfiltered, + private Iterator> filterDeletedValues( + Iterator> unfiltered, TableReference tableReference) { Meter emptyValueMeter = getMeter(AtlasDbMetricNames.CellFilterMetrics.EMPTY_VALUE, tableReference); return Iterators.filter(unfiltered, entry -> { @@ -442,22 +479,22 @@ private Iterator> filterDeletedValues( }); } - private Iterator> getRowColumnRangePostFiltered( + private Iterator> getRowColumnRangePostFiltered( TableReference tableRef, byte[] row, BatchColumnRangeSelection columnRangeSelection, RowColumnRangeIterator rawIterator) { ColumnRangeBatchProvider batchProvider = new ColumnRangeBatchProvider( keyValueService, tableRef, row, columnRangeSelection, getStartTimestamp()); - BatchSizeIncreasingIterator> batchIterator = new BatchSizeIncreasingIterator<>( + BatchSizeIncreasingIterator> batchIterator = new BatchSizeIncreasingIterator<>( batchProvider, columnRangeSelection.getBatchHint(), ClosableIterators.wrap(rawIterator)); - Iterator>> postFilteredBatches = - new AbstractIterator>>() { + Iterator>> postFilteredBatches = + new AbstractIterator>>() { @Override - protected Iterator> computeNext() { + protected Iterator> computeNext() { ImmutableMap.Builder rawBuilder = ImmutableMap.builder(); - List> batch = batchIterator.getBatch(); - for (Map.Entry result : batch) { + List> batch = batchIterator.getBatch(); + for (Entry result : batch) { rawBuilder.put(result); } Map raw = rawBuilder.build(); @@ -481,22 +518,22 @@ protected Iterator> computeNext() { * that all columns for a single row are adjacent, so this method will return an {@link Iterator} with exactly one * entry per non-empty row. */ - private Iterator> partitionByRow(RowColumnRangeIterator rawResults) { - PeekingIterator> peekableRawResults = Iterators.peekingIterator(rawResults); - return new AbstractIterator>() { + private Iterator> partitionByRow(RowColumnRangeIterator rawResults) { + PeekingIterator> peekableRawResults = Iterators.peekingIterator(rawResults); + return new AbstractIterator>() { byte[] prevRowName; @Override - protected Map.Entry computeNext() { + protected Entry computeNext() { finishConsumingPreviousRow(peekableRawResults); if (!peekableRawResults.hasNext()) { return endOfData(); } byte[] nextRowName = peekableRawResults.peek().getKey().getRowName(); - Iterator> columnsIterator = - new AbstractIterator>() { + Iterator> columnsIterator = + new AbstractIterator>() { @Override - protected Map.Entry computeNext() { + protected Entry computeNext() { if (!peekableRawResults.hasNext() || !Arrays.equals(peekableRawResults.peek().getKey().getRowName(), nextRowName)) { return endOfData(); @@ -508,7 +545,7 @@ protected Map.Entry computeNext() { return Maps.immutableEntry(nextRowName, new LocalRowColumnRangeIterator(columnsIterator)); } - private void finishConsumingPreviousRow(PeekingIterator> iter) { + private void finishConsumingPreviousRow(PeekingIterator> iter) { int numConsumed = 0; while (iter.hasNext() && Arrays.equals(iter.peek().getKey().getRowName(), prevRowName)) { iter.next(); @@ -679,6 +716,23 @@ private interface CellLoader { ListenableFuture> load(TableReference tableReference, Map toRead); } + /** + * This will load the given keys from the underlying key value service and apply postFiltering + * so we have snapshot isolation. If the value in the key value service is the empty array + * this will be included here and needs to be filtered out. + */ + private ListenableFuture> getFromKeyValueServiceAsync(TableReference tableRef, Set cells) { + ImmutableMap.Builder result = ImmutableMap.builderWithExpectedSize(cells.size()); + Map toRead = Cells.constantValueMap(cells, getStartTimestamp()); + return Futures.transform( + keyValueService.getAsync(tableRef, toRead), + rawResults -> { + getWithPostFiltering(tableRef, rawResults, result, Value.GET_VALUE); + return result.build(); + }, + MoreExecutors.directExecutor()); + } + private static byte[] getNextStartRowName( RangeRequest range, TokenBackedBasicResultsPage, byte[]> prePostFilter) { @@ -1093,7 +1147,7 @@ private SortedMap postFilterRows(TableReference tableRef, Map rawResults = Maps.newHashMapWithExpectedSize(estimateSize(rangeRows)); for (RowResult rowResult : rangeRows) { - for (Map.Entry e : rowResult.getColumns().entrySet()) { + for (Entry e : rowResult.getColumns().entrySet()) { rawResults.put(Cell.create(rowResult.getRowName(), e.getKey()), e.getValue()); } } @@ -1116,7 +1170,7 @@ private void getWithPostFiltering(TableReference tableRef, @Output ImmutableMap.Builder results, Function transformer) { long bytes = 0; - for (Map.Entry e : rawResults.entrySet()) { + for (Entry e : rawResults.entrySet()) { bytes += e.getValue().getContents().length + Cells.getApproxSizeOfCell(e.getKey()); } if (bytes > TransactionConstants.WARN_LEVEL_FOR_QUEUED_BYTES && log.isWarnEnabled()) { @@ -1140,7 +1194,7 @@ private void getWithPostFiltering(TableReference tableRef, // hidden tables are used outside of the transaction protocol, and in general have invalid timestamps, // so do not apply post-filtering as post-filtering would rollback (actually delete) the data incorrectly // this case is hit when reading a hidden table from console - for (Map.Entry e : rawResults.entrySet()) { + for (Entry e : rawResults.entrySet()) { results.put(e.getKey(), transformer.apply(e.getValue())); } return; @@ -1192,7 +1246,7 @@ private Map getWithPostFilteringInternal(TableReference tableRe ImmutableSet.Builder keysAddedBuilder = ImmutableSet.builder(); Set orphanedSentinels = findOrphanedSweepSentinels(tableRef, rawResults); - for (Map.Entry e : rawResults.entrySet()) { + for (Entry e : rawResults.entrySet()) { Cell key = e.getKey(); Value value = e.getValue(); @@ -1319,7 +1373,7 @@ private void ensureNoEmptyValues(Map values) { } private void putWritesAndLogIfTooLarge(Map values, SortedMap writes) { - for (Map.Entry e : values.entrySet()) { + for (Entry e : values.entrySet()) { byte[] val = MoreObjects.firstNonNull(e.getValue(), PtBytes.EMPTY_BYTE_ARRAY); Cell cell = e.getKey(); if (writes.put(cell, val) == null) { @@ -1434,7 +1488,7 @@ public void commit(TransactionService transactionService) { private void checkConstraints() { List violations = Lists.newArrayList(); - for (Map.Entry entry : constraintsByTableName.entrySet()) { + for (Entry entry : constraintsByTableName.entrySet()) { SortedMap sortedMap = writesByTable.get(entry.getKey()); if (sortedMap != null) { violations.addAll(entry.getValue().findConstraintFailures(sortedMap, this, constraintCheckingMode)); @@ -1735,7 +1789,7 @@ protected Map detectWriteAlreadyCommittedInternal(TableReference tab Map commitTimestamps = getCommitTimestamps(tableRef, rawResults.values(), false); Map keysToDelete = Maps.newHashMapWithExpectedSize(0); - for (Map.Entry e : rawResults.entrySet()) { + for (Entry e : rawResults.entrySet()) { Cell key = e.getKey(); long theirStartTimestamp = e.getValue(); AssertUtils.assertAndLog(log, theirStartTimestamp != getStartTimestamp(), @@ -2023,7 +2077,7 @@ protected Map getCommitTimestamps(@Nullable TableReference tableRef, Map rawResults = loadCommitTimestamps(gets); - for (Map.Entry e : rawResults.entrySet()) { + for (Entry e : rawResults.entrySet()) { if (e.getValue() != null) { long startTs = e.getKey(); long commitTs = e.getValue(); From fd824ceefdc316f457951de348e2ed01a86728be Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 10:48:04 +0100 Subject: [PATCH 03/16] Initial transactions test --- ...ssandraKvsSerializableTransactionTest.java | 73 ++++++++++++++++++- .../impl/SerializableTransaction.java | 10 +++ .../transaction/impl/SnapshotTransaction.java | 13 +++- 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java index 8d4d4de303c..8befd020175 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java @@ -17,22 +17,93 @@ import static org.mockito.Mockito.mock; +import java.util.Arrays; +import java.util.Collection; +import java.util.Map; +import java.util.Set; +import java.util.function.Function; + import org.junit.ClassRule; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import com.palantir.atlasdb.containers.CassandraResource; +import com.palantir.atlasdb.keyvalue.api.Cell; +import com.palantir.atlasdb.keyvalue.api.TableReference; import com.palantir.atlasdb.sweep.metrics.TargetedSweepMetrics; import com.palantir.atlasdb.sweep.queue.MultiTableSweepQueueWriter; import com.palantir.atlasdb.sweep.queue.SweepQueue; import com.palantir.atlasdb.sweep.queue.TargetedSweeper; +import com.palantir.atlasdb.transaction.api.Transaction; import com.palantir.atlasdb.transaction.impl.AbstractSerializableTransactionTest; +import com.palantir.atlasdb.transaction.impl.ForwardingTransaction; +@RunWith(Parameterized.class) public class CassandraKvsSerializableTransactionTest extends AbstractSerializableTransactionTest { @ClassRule public static final CassandraResource CASSANDRA = new CassandraResource(); + private static class SynchronousDelegate extends ForwardingTransaction { + private final Transaction delegate; + + SynchronousDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + return delegate.get(tableRef, cells); + } + } + + private static class AsyncDelegate extends ForwardingTransaction { + private final Transaction delegate; + + AsyncDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } - public CassandraKvsSerializableTransactionTest() { + @Override + public Map get(TableReference tableRef, Set cells) { + try { + return delegate.getAsync(tableRef, cells).get(); + } catch (Exception e) { + throw new RuntimeException(e.getCause()); + } + } + } + + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Object[][] data = new Object[][] { + {"sync", (Function) SynchronousDelegate::new}, + {"async", (Function) AsyncDelegate::new} + }; + return Arrays.asList(data); + } + + private final Function transactionWrapper; + + public CassandraKvsSerializableTransactionTest( + String name, + Function transactionWrapper) { super(CASSANDRA, CASSANDRA); + this.transactionWrapper = transactionWrapper; + } + + @Override + protected Transaction startTransaction() { + return transactionWrapper.apply(super.startTransaction()); } @Override diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java index 9abb7616326..7462b8d03d8 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java @@ -276,6 +276,16 @@ private interface CellLoader { ListenableFuture> load(TableReference tableReference, Set toRead); } + @Override + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + return Futures.transform(super.getAsync(tableRef, cells), + ret -> { + markCellsRead(tableRef, cells, ret); + return ret; + }, + MoreExecutors.directExecutor()); + } + @Override @Idempotent public BatchingVisitable> getRange(TableReference tableRef, RangeRequest rangeRequest) { diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index 6e0b2e85e31..a780a0100e7 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -323,21 +323,26 @@ public ListenableFuture> getAsync(TableReference tableRef, Set } hasReads = true; - Map result = Maps.newHashMap(); + ImmutableMap.Builder resultBuilder = ImmutableMap.builder(); SortedMap writes = writesByTable.get(tableRef); if (writes != null) { for (Cell cell : cells) { if (writes.containsKey(cell)) { - result.put(cell, writes.get(cell)); + resultBuilder.put(cell, writes.get(cell)); } } } + Map writtenLocally = resultBuilder.build(); + return Futures.transform( // We don't need to read any cells that were written locally. - getFromKeyValueServiceAsync(tableRef, Sets.difference(cells, result.keySet())), + getFromKeyValueServiceAsync(tableRef, Sets.difference(cells, writtenLocally.keySet())), keyValueResult -> { - result.putAll(keyValueResult); + ImmutableMap result = ImmutableMap.builder() + .putAll(writtenLocally) + .putAll(keyValueResult) + .build(); long getMillis = TimeUnit.NANOSECONDS.toMillis(timer.stop()); if (perfLogger.isDebugEnabled()) { From 9d789fc153abb1de88e34c5d906e65aa666a99db Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 11:07:05 +0100 Subject: [PATCH 04/16] Caching transaction getAsync. --- .../transaction/impl/CachingTransaction.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java index e4aae17cfed..1004e655dd3 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java @@ -155,6 +155,37 @@ private ListenableFuture> get(TableReference tableRef, Set> getAsync(TableReference tableRef, Set cells) { + if (cells.isEmpty()) { + return Futures.immediateFuture(ImmutableMap.of()); + } + + Set toLoad = Sets.newHashSet(); + ImmutableMap.Builder cacheHitMapBuilder = ImmutableMap.builder(); + for (Cell cell : cells) { + byte[] val = getCachedCellIfPresent(tableRef, cell); + if (val != null) { + if (val.length > 0) { + cacheHitMapBuilder.put(cell, val); + } + } else { + toLoad.add(cell); + } + } + + ImmutableMap cacheHit = cacheHitMapBuilder.build(); + + return Futures.transform(super.getAsync(tableRef, toLoad), + loaded -> { + cacheLoadedCells(tableRef, toLoad, loaded); + return ImmutableMap.builder() + .putAll(loaded.entrySet()) + .putAll(cacheHit.entrySet()) + .build(); + }, MoreExecutors.directExecutor()); + } + @Override public final void delete(TableReference tableRef, Set cells) { super.delete(tableRef, cells); From 30332a8ad7c54d2e95fb75519a4c2b74b58c9611 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 13:55:37 +0100 Subject: [PATCH 05/16] Retrofitted tests. --- ...alueServiceTransactionIntegrationTest.java | 75 ++++++- ...ssandraKvsSerializableTransactionTest.java | 79 ++++--- .../impl/CachingTransactionTest.java | 86 ++++++- .../impl/SnapshotTransactionTest.java | 209 +++++++++++++----- 4 files changed, 352 insertions(+), 97 deletions(-) diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java index db51720ec2b..9201a800098 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java @@ -15,10 +15,15 @@ */ package com.palantir.atlasdb.keyvalue.cassandra; +import java.util.Arrays; +import java.util.Collection; import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.LongStream; @@ -28,13 +33,19 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestRule; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import com.google.common.base.Suppliers; import com.google.common.util.concurrent.Futures; import com.palantir.atlasdb.containers.CassandraResource; +import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.KeyAlreadyExistsException; import com.palantir.atlasdb.keyvalue.api.KeyValueService; +import com.palantir.atlasdb.keyvalue.api.TableReference; +import com.palantir.atlasdb.transaction.api.Transaction; import com.palantir.atlasdb.transaction.impl.AbstractTransactionTest; +import com.palantir.atlasdb.transaction.impl.ForwardingTransaction; import com.palantir.atlasdb.transaction.impl.TransactionTables; import com.palantir.atlasdb.transaction.service.SimpleTransactionService; import com.palantir.atlasdb.transaction.service.TransactionService; @@ -44,10 +55,22 @@ import com.palantir.timestamp.TimestampManagementService; @ShouldRetry // The first test can fail with a TException: No host tried was able to create the keyspace requested. +@RunWith(Parameterized.class) public class CassandraKeyValueServiceTransactionIntegrationTest extends AbstractTransactionTest { + private static final String SYNC = "sync"; + private static final String ASYNC = "async"; private static final Supplier KVS_SUPPLIER = Suppliers.memoize(CassandraKeyValueServiceTransactionIntegrationTest::createAndRegisterKeyValueService); + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Object[][] data = new Object[][] { + {SYNC, (Function) SynchronousDelegate::new}, + {ASYNC, (Function) AsyncDelegate::new} + }; + return Arrays.asList(data); + } + @ClassRule public static final CassandraResource CASSANDRA = new CassandraResource(KVS_SUPPLIER); @@ -57,9 +80,15 @@ public class CassandraKeyValueServiceTransactionIntegrationTest extends Abstract @Rule public final TestRule flakeRetryingRule = new FlakeRetryingRule(); + private final String name; + private final Function transactionWrapper; - public CassandraKeyValueServiceTransactionIntegrationTest() { + public CassandraKeyValueServiceTransactionIntegrationTest( + String name, + Function transactionWrapper) { super(CASSANDRA, CASSANDRA); + this.name = name; + this.transactionWrapper = transactionWrapper; } @Before @@ -106,4 +135,48 @@ protected boolean supportsReverse() { return false; } + @Override + protected Transaction startTransaction() { + return transactionWrapper.apply(super.startTransaction()); + } + + private static class SynchronousDelegate extends ForwardingTransaction { + private final Transaction delegate; + + SynchronousDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + return delegate.get(tableRef, cells); + } + } + + private static class AsyncDelegate extends ForwardingTransaction { + private final Transaction delegate; + + AsyncDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + try { + return delegate.getAsync(tableRef, cells).get(); + } catch (Exception e) { + throw new RuntimeException(e.getCause()); + } + } + } } diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java index 8befd020175..a8e8378ed38 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java @@ -43,46 +43,6 @@ public class CassandraKvsSerializableTransactionTest extends AbstractSerializabl @ClassRule public static final CassandraResource CASSANDRA = new CassandraResource(); - private static class SynchronousDelegate extends ForwardingTransaction { - private final Transaction delegate; - - SynchronousDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - return delegate.get(tableRef, cells); - } - } - - private static class AsyncDelegate extends ForwardingTransaction { - private final Transaction delegate; - - AsyncDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - try { - return delegate.getAsync(tableRef, cells).get(); - } catch (Exception e) { - throw new RuntimeException(e.getCause()); - } - } - } - @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { @@ -126,4 +86,43 @@ protected boolean supportsReverse() { return false; } + private static class SynchronousDelegate extends ForwardingTransaction { + private final Transaction delegate; + + SynchronousDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + return delegate.get(tableRef, cells); + } + } + + private static class AsyncDelegate extends ForwardingTransaction { + private final Transaction delegate; + + AsyncDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + try { + return delegate.getAsync(tableRef, cells).get(); + } catch (Exception e) { + throw new RuntimeException(e.getCause()); + } + } + } } diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java index bed5c97b5e8..aa222429112 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java @@ -15,20 +15,27 @@ */ package com.palantir.atlasdb.transaction.impl; +import java.util.Arrays; +import java.util.Collection; import java.util.Map; import java.util.Set; import java.util.SortedMap; +import java.util.function.BiFunction; +import java.util.function.Function; import org.jmock.Expectations; import org.jmock.Mockery; import org.junit.Assert; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.util.concurrent.Futures; import com.palantir.atlasdb.encoding.PtBytes; import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.ColumnSelection; @@ -36,15 +43,38 @@ import com.palantir.atlasdb.keyvalue.api.TableReference; import com.palantir.atlasdb.transaction.api.Transaction; +@RunWith(Parameterized.class) public class CachingTransactionTest { private static final byte[] ROW_BYTES = "row".getBytes(); private static final byte[] COL_BYTES = "col".getBytes(); private static final byte[] VALUE_BYTES = "value".getBytes(); + private static final String SYNC = "sync"; + private static final String ASYNC = "async"; + + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Object[][] data = new Object[][] { + {SYNC, (Function) SynchronousDelegate::new}, + {ASYNC, (Function) AsyncDelegate::new} + }; + return Arrays.asList(data); + } private final TableReference table = TableReference.createWithEmptyNamespace("table"); private final Mockery mockery = new Mockery(); private final Transaction txn = mockery.mock(Transaction.class); - private final CachingTransaction ct = new CachingTransaction(txn); + private final CachingTransaction ct; + private final String name; + private final Map, Map, Expectations>> expectationsMapping = + ImmutableMap., Map, Expectations>>builder() + .put(SYNC, CachingTransactionTest.this::syncGetExpectation) + .put(ASYNC, CachingTransactionTest.this::asyncGetExpectation) + .build(); + + public CachingTransactionTest(String name, Function transactionWrapper) { + this.name = name; + ct = transactionWrapper.apply(new CachingTransaction(txn)); + } @Test public void testCacheEmptyGets() { @@ -125,7 +155,16 @@ public void testGetEmptyCell() { private void testGetCellResults(Cell cell, Map cellValueMap) { final Set cellSet = ImmutableSet.of(cell); - mockery.checking(new Expectations() { + mockery.checking(expectationsMapping.get(name).apply(cellSet, cellValueMap)); + + Assert.assertEquals(cellValueMap, ct.get(table, cellSet)); + Assert.assertEquals(cellValueMap, ct.get(table, cellSet)); + + mockery.assertIsSatisfied(); + } + + private Expectations syncGetExpectation(Set cellSet, Map cellValueMap) { + return new Expectations() { { oneOf(txn).get(table, cellSet); will(returnValue(cellValueMap)); @@ -133,11 +172,46 @@ private void testGetCellResults(Cell cell, Map cellValueMap) { oneOf(txn).get(table, ImmutableSet.of()); will(returnValue(ImmutableMap.of())); } - }); + }; + } - Assert.assertEquals(cellValueMap, ct.get(table, cellSet)); - Assert.assertEquals(cellValueMap, ct.get(table, cellSet)); + private Expectations asyncGetExpectation(Set cellSet, Map cellValueMap) { + return new Expectations() { + { + oneOf(txn).getAsync(table, cellSet); + will(returnValue(Futures.immediateFuture(cellValueMap))); - mockery.assertIsSatisfied(); + oneOf(txn).getAsync(table, ImmutableSet.of()); + will(returnValue(Futures.immediateFuture(ImmutableMap.of()))); + } + }; + } + + private static class SynchronousDelegate extends CachingTransaction { + + SynchronousDelegate(CachingTransaction transaction) { + super(transaction.delegate()); + } + + @Override + public Map get(TableReference tableRef, Set cells) { + return super.get(tableRef, cells); + } + } + + private static class AsyncDelegate extends CachingTransaction { + + AsyncDelegate(CachingTransaction transaction) { + super(transaction.delegate()); + } + + @Override + public Map get(TableReference tableRef, Set cells) { + try { + return super.getAsync(tableRef, cells).get(); + } catch (Exception e) { + throw new RuntimeException(e.getCause()); + } + } } } diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index ffb38586deb..c350489b573 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -37,6 +37,7 @@ import static org.mockito.Mockito.when; import java.math.BigInteger; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Iterator; @@ -44,6 +45,7 @@ import java.util.Map; import java.util.Optional; import java.util.Random; +import java.util.Set; import java.util.SortedMap; import java.util.concurrent.Callable; import java.util.concurrent.CompletionService; @@ -54,6 +56,7 @@ import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -70,6 +73,8 @@ import org.junit.Before; import org.junit.Ignore; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import com.google.common.base.Joiner; import com.google.common.base.Suppliers; @@ -105,6 +110,7 @@ import com.palantir.atlasdb.transaction.ImmutableTransactionConfig; import com.palantir.atlasdb.transaction.TransactionConfig; import com.palantir.atlasdb.transaction.api.AtlasDbConstraintCheckingMode; +import com.palantir.atlasdb.transaction.api.AutoDelegate_TransactionManager; import com.palantir.atlasdb.transaction.api.ConflictHandler; import com.palantir.atlasdb.transaction.api.LockAwareTransactionTask; import com.palantir.atlasdb.transaction.api.PreCommitCondition; @@ -115,6 +121,7 @@ import com.palantir.atlasdb.transaction.api.TransactionFailedRetriableException; import com.palantir.atlasdb.transaction.api.TransactionLockTimeoutException; import com.palantir.atlasdb.transaction.api.TransactionLockTimeoutNonRetriableException; +import com.palantir.atlasdb.transaction.api.TransactionManager; import com.palantir.atlasdb.transaction.api.TransactionReadSentinelBehavior; import com.palantir.atlasdb.transaction.impl.metrics.TransactionOutcomeMetrics; import com.palantir.atlasdb.transaction.impl.metrics.TransactionOutcomeMetricsAssert; @@ -140,7 +147,10 @@ import com.palantir.timestamp.TimestampService; @SuppressWarnings("checkstyle:all") +@RunWith(Parameterized.class) public class SnapshotTransactionTest extends AtlasDbTestCase { + private final String name; + private final Function transactionWrapper; private TransactionConfig transactionConfig; protected final TimestampCache timestampCache = new DefaultTimestampCache( @@ -149,6 +159,21 @@ public class SnapshotTransactionTest extends AtlasDbTestCase { protected final int defaultGetRangesConcurrency = 2; protected final TransactionOutcomeMetrics transactionOutcomeMetrics = TransactionOutcomeMetrics.create(metricsManager); + private WrappingTestTransactionManager wrappedSerializableTxManager; + + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Object[][] data = new Object[][] { + {"sync", (Function) SynchronousDelegate::new}, + {"async", (Function) AsyncDelegate::new} + }; + return Arrays.asList(data); + } + + public SnapshotTransactionTest(String name, Function transactionWrapper) { + this.name = name; + this.transactionWrapper = transactionWrapper; + } private class UnstableKeyValueService implements AutoDelegate_KeyValueService { private final KeyValueService delegate; @@ -217,6 +242,9 @@ public void cleanup() {} public void setUp() throws Exception { super.setUp(); + txManager = new WrappingTestTransactionManager(txManager, transactionWrapper); + wrappedSerializableTxManager = new WrappingTestTransactionManager(serializableTxManager, transactionWrapper); + transactionConfig = ImmutableTransactionConfig.builder().build(); keyValueService.createTable(TABLE, AtlasDbConstants.GENERIC_TABLE_METADATA); @@ -283,11 +311,12 @@ public void testLockAfterGet() throws Exception { keyValueService.put(TABLE, ImmutableMap.of(cell, PtBytes.EMPTY_BYTE_ARRAY), startTs); m.checking(new Expectations() {{ - oneOf(kvMock).get(TABLE, ImmutableMap.of(cell, transactionTs)); will(throwException(new RuntimeException())); + oneOf(kvMock).get(TABLE, ImmutableMap.of(cell, transactionTs)); + will(throwException(new RuntimeException())); never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); }}); - SnapshotTransaction snapshot = new SnapshotTransaction(metricsManager, + Transaction snapshot = transactionWrapper.apply(new SnapshotTransaction(metricsManager, kvMock, new LegacyTimelockService(timestampService, lock, lockClient), transactionService, @@ -308,7 +337,7 @@ public void testLockAfterGet() throws Exception { MultiTableSweepQueueWriter.NO_OP, MoreExecutors.newDirectExecutorService(), true, - () -> transactionConfig); + () -> transactionConfig)); try { snapshot.get(TABLE, ImmutableSet.of(cell)); fail(); @@ -353,7 +382,7 @@ public void testPutCleanup() throws Exception { inSequence(seq); }}); - SnapshotTransaction snapshot = new SnapshotTransaction(metricsManager, + Transaction snapshot = transactionWrapper.apply(new SnapshotTransaction(metricsManager, keyValueService, null, transactionService, @@ -374,7 +403,7 @@ public void testPutCleanup() throws Exception { MultiTableSweepQueueWriter.NO_OP, MoreExecutors.newDirectExecutorService(), true, - () -> transactionConfig); + () -> transactionConfig)); snapshot.delete(TABLE, ImmutableSet.of(cell)); snapshot.commit(); @@ -391,18 +420,20 @@ public void testTransactionAtomicity() throws Exception { Random random = new Random(1); final UnstableKeyValueService unstableKvs = new UnstableKeyValueService(keyValueService, random); - final TestTransactionManager unstableTransactionManager = new TestTransactionManagerImpl( - metricsManager, - unstableKvs, - timestampService, - timestampService, - lockClient, - lockService, - transactionService, - conflictDetectionManager, - sweepStrategyManager, - sweepQueue, - MoreExecutors.newDirectExecutorService()); + final TestTransactionManager unstableTransactionManager = new WrappingTestTransactionManager( + new TestTransactionManagerImpl( + metricsManager, + unstableKvs, + timestampService, + timestampService, + lockClient, + lockService, + transactionService, + conflictDetectionManager, + sweepStrategyManager, + sweepQueue, + MoreExecutors.newDirectExecutorService()), + transactionWrapper); ScheduledExecutorService service = PTExecutors.newScheduledThreadPool(20); @@ -532,7 +563,7 @@ public void testGetRowsLocalWritesWithColumnSelection() { byte[] row1Column1Value = BigInteger.valueOf(1).toByteArray(); byte[] row1Column2Value = BigInteger.valueOf(2).toByteArray(); - Transaction snapshotTx = serializableTxManager.createNewTransaction(); + Transaction snapshotTx = wrappedSerializableTxManager.createNewTransaction(); snapshotTx.put(TABLE, ImmutableMap.of( row1Column1, row1Column1Value, row1Column2, row1Column2Value)); @@ -618,7 +649,7 @@ public void readsFromThoroughlySweptTableShouldFailWhenLocksAreInvalid() { .collect(Collectors.toList()); if (!successfulTasks.isEmpty()) { Assert.fail("Expected read to fail with TransactionFailedRetriableException, but it succeeded for: " + - Joiner.on(", ").join(successfulTasks)); + Joiner.on(", ").join(successfulTasks)); } } @@ -642,7 +673,8 @@ private List>> getThoroug final int batchHint = 1; tasks.add(Pair.of("get", (t, heldLocks) -> { - t.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(Cell.create(PtBytes.toBytes("row1"), PtBytes.toBytes("column1")))); + t.get(TABLE_SWEPT_THOROUGH, + ImmutableSet.of(Cell.create(PtBytes.toBytes("row1"), PtBytes.toBytes("column1")))); return null; })); @@ -682,7 +714,8 @@ private List>> getThoroug tasks.add(Pair.of("getRowsColumnRangeIterator", (t, heldLocks) -> { Collection>> results = - t.getRowsColumnRangeIterator(TABLE_SWEPT_THOROUGH, Collections.singleton(PtBytes.toBytes("row1")), + t.getRowsColumnRangeIterator(TABLE_SWEPT_THOROUGH, + Collections.singleton(PtBytes.toBytes("row1")), BatchColumnRangeSelection.create(new ColumnRangeSelection(null, null), batchHint)) .values(); results.forEach(Iterators::getLast); @@ -791,7 +824,7 @@ public void testWriteWriteConflictsDeletedThrow() { } } - @Test (expected = IllegalArgumentException.class) + @Test(expected = IllegalArgumentException.class) public void disallowPutOnEmptyObject() { Transaction t1 = txManager.createNewTransaction(); t1.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.EMPTY_BYTE_ARRAY)); @@ -829,7 +862,7 @@ public void noRetryOnExpiredLockTokens() throws InterruptedException { @Test public void commitIfPreCommitConditionSucceeds() { - serializableTxManager.runTaskWithConditionThrowOnConflict(PreCommitConditions.NO_OP, (tx, condition) -> { + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(PreCommitConditions.NO_OP, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; }); @@ -838,10 +871,11 @@ public void commitIfPreCommitConditionSucceeds() { @Test public void failToCommitIfPreCommitConditionFails() { try { - serializableTxManager.runTaskWithConditionThrowOnConflict(ALWAYS_FAILS_CONDITION, (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(ALWAYS_FAILS_CONDITION, + (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); fail(); } catch (TransactionFailedRetriableException e) { assertThat(e.getMessage(), containsString("Condition failed")); @@ -854,7 +888,7 @@ public void commitWithPreCommitConditionOnRetry() { when(conditionSupplier.get()).thenReturn(ALWAYS_FAILS_CONDITION) .thenReturn(PreCommitConditions.NO_OP); - serializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { + wrappedSerializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; }); @@ -908,7 +942,7 @@ public void cleanup() {} }; Supplier conditionSupplier = Suppliers.ofInstance(nonRetriableFailure); try { - serializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { + wrappedSerializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; }); @@ -920,7 +954,7 @@ public void cleanup() {} @Test public void readTransactionSucceedsIfConditionSucceeds() { - serializableTxManager.runTaskWithConditionReadOnly(PreCommitConditions.NO_OP, + wrappedSerializableTxManager.runTaskWithConditionReadOnly(PreCommitConditions.NO_OP, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); TransactionOutcomeMetricsAssert.assertThat(transactionOutcomeMetrics) .hasSuccessfulCommits(1); @@ -929,7 +963,7 @@ public void readTransactionSucceedsIfConditionSucceeds() { @Test public void readTransactionFailsIfConditionFails() { try { - serializableTxManager.runTaskWithConditionReadOnly(ALWAYS_FAILS_CONDITION, + wrappedSerializableTxManager.runTaskWithConditionReadOnly(ALWAYS_FAILS_CONDITION, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); fail(); } catch (TransactionFailedRetriableException e) { @@ -952,13 +986,13 @@ public void cleanup() { } }; - serializableTxManager.runTaskWithConditionThrowOnConflict(succeedsCondition, (tx, condition) -> { + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(succeedsCondition, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; }); assertThat(counter.intValue(), is(1)); - serializableTxManager.runTaskWithConditionReadOnly(succeedsCondition, + wrappedSerializableTxManager.runTaskWithConditionReadOnly(succeedsCondition, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); assertThat(counter.intValue(), is(2)); } @@ -979,7 +1013,7 @@ public void cleanup() { }; try { - serializableTxManager.runTaskWithConditionThrowOnConflict(failsCondition, (tx, condition) -> { + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(failsCondition, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; }); @@ -990,7 +1024,7 @@ public void cleanup() { assertThat(counter.intValue(), is(1)); try { - serializableTxManager.runTaskWithConditionReadOnly(failsCondition, + wrappedSerializableTxManager.runTaskWithConditionReadOnly(failsCondition, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); fail(); } catch (TransactionFailedRetriableException e) { @@ -1006,27 +1040,28 @@ public void getRowsColumnRangesReturnsInOrderInCaseOfAbortedTxns() { Cell secondCell = Cell.create(row, "b".getBytes()); byte[] value = new byte[1]; - serializableTxManager.runTaskWithRetry(tx -> { + wrappedSerializableTxManager.runTaskWithRetry(tx -> { tx.put(TABLE, ImmutableMap.of(firstCell, value, secondCell, value)); return null; }); // this will write into the DB, because the protocol demands we write before we get a commit timestamp RuntimeException conditionFailure = new RuntimeException(); - assertThatThrownBy(() -> serializableTxManager.runTaskWithConditionWithRetry(() -> new PreCommitCondition() { - @Override - public void throwIfConditionInvalid(long timestamp) { - throw conditionFailure; - } - - @Override - public void cleanup() {} - }, (tx, condition) -> { + assertThatThrownBy(() -> wrappedSerializableTxManager.runTaskWithConditionWithRetry(() -> + new PreCommitCondition() { + @Override + public void throwIfConditionInvalid(long timestamp) { + throw conditionFailure; + } + + @Override + public void cleanup() {} + }, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(firstCell, value)); return null; })).isSameAs(conditionFailure); - List cells = serializableTxManager.runTaskReadOnly(tx -> + List cells = wrappedSerializableTxManager.runTaskReadOnly(tx -> BatchingVisitableView.of(tx.getRowsColumnRange( TABLE, ImmutableList.of(row), @@ -1139,7 +1174,7 @@ public void validateLocksOnReadsIfThoroughlySwept() { timelockService.unlock(ImmutableSet.of(res.getLock())); assertThatExceptionOfType(TransactionLockTimeoutException.class).isThrownBy(() -> - transaction.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(TEST_CELL))); + transaction.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(TEST_CELL))); } @Test @@ -1179,7 +1214,7 @@ public void checkImmutableTsLockOnceIfThoroughlySwept_WithValidationOnReads() { transaction.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(TEST_CELL)); transaction.commit(); timelockService.unlock(ImmutableSet.of(res.getLock())); - + verify(timelockService).refreshLockLeases(ImmutableSet.of(res.getLock())); } @@ -1250,7 +1285,7 @@ public void checkImmutableTsLockAfterReadsForConservativeIfFlagIsSet() { transaction.get(TABLE_SWEPT_CONSERVATIVE, ImmutableSet.of(TEST_CELL)); verify(timelockService).refreshLockLeases(ImmutableSet.of(res.getLock())); - + transaction.commit(); timelockService.unlock(ImmutableSet.of(res.getLock())); @@ -1313,7 +1348,8 @@ private void writeCells(TableReference table, ImmutableMap cellsTo private RowResult readRow(byte[] defaultRow) { Transaction readTransaction = txManager.createNewTransaction(); - SortedMap> allRows = readTransaction.getRows(TABLE, ImmutableSet.of(defaultRow), ColumnSelection.all()); + SortedMap> allRows = readTransaction.getRows(TABLE, ImmutableSet.of(defaultRow), + ColumnSelection.all()); return allRows.get(defaultRow); } @@ -1378,4 +1414,77 @@ private static SnapshotTransaction unwrapSnapshotTransaction(Transaction caching return (SnapshotTransaction) unwrapped; } + private static class SynchronousDelegate extends ForwardingTransaction { + private final Transaction delegate; + + SynchronousDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + return delegate.get(tableRef, cells); + } + } + + private static class AsyncDelegate extends ForwardingTransaction { + private final Transaction delegate; + + AsyncDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + try { + return delegate.getAsync(tableRef, cells).get(); + } catch (Exception e) { + throw new RuntimeException(e.getCause()); + } + } + } + + private static class WrappingTestTransactionManager + implements TestTransactionManager, AutoDelegate_TransactionManager { + + private final TestTransactionManager testTransactionManager; + private final Function transactionWrapper; + + WrappingTestTransactionManager( + TestTransactionManager testTransactionManager, + Function transactionWrapper) { + this.testTransactionManager = testTransactionManager; + this.transactionWrapper = transactionWrapper; + } + + @Override + public TransactionManager delegate() { + return testTransactionManager; + } + + @Override + public Transaction commitAndStartNewTransaction(Transaction txn) { + return testTransactionManager.commitAndStartNewTransaction(txn); + } + + @Override + public Transaction createNewTransaction() { + return transactionWrapper.apply(testTransactionManager.createNewTransaction()); + } + + @Override + public void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler) { + testTransactionManager.overrideConflictHandlerForTable(table, conflictHandler); + } + } } From 16981e25e7212b4e54c17977d97a27bea2629b1c Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 14:52:55 +0100 Subject: [PATCH 06/16] Fixed tests. --- .../impl/SnapshotTransactionTest.java | 65 +++++++++++++++---- 1 file changed, 52 insertions(+), 13 deletions(-) diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index c350489b573..d6e3d265a7f 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -87,6 +87,7 @@ import com.google.common.collect.Iterators; import com.google.common.collect.Lists; import com.google.common.collect.Multimaps; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.MoreExecutors; import com.palantir.atlasdb.AtlasDbConstants; import com.palantir.atlasdb.AtlasDbTestCase; @@ -149,23 +150,65 @@ @SuppressWarnings("checkstyle:all") @RunWith(Parameterized.class) public class SnapshotTransactionTest extends AtlasDbTestCase { + private static final String SYNC = "sync"; + private static final String ASYNC = "async"; private final String name; private final Function transactionWrapper; + private final Map> + expectationsMapping = + ImmutableMap.>builder() + .put(SYNC, SnapshotTransactionTest.this::syncGetExpectation) + .put(ASYNC, SnapshotTransactionTest.this::asyncGetExpectation) + .build(); + + @FunctionalInterface + interface ExpectationFactory { + Five apply(One one, Two two, Three three, Four four) throws Exception; + } + + private Expectations syncGetExpectation( + KeyValueService kvMock, + Cell cell, + long transactionTs, + LockService lockMock) { + try { + return new Expectations() {{ + oneOf(kvMock).get(TABLE, ImmutableMap.of(cell, transactionTs)); + will(throwException(new RuntimeException())); + never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); + }}; + } catch (Exception e) { + throw new RuntimeException(e.getCause()); + } + } + + private Expectations asyncGetExpectation( + KeyValueService kvMock, + Cell cell, + long transactionTs, + LockService lockMock) throws Exception { + return new Expectations() {{ + oneOf(kvMock).getAsync(TABLE, ImmutableMap.of(cell, transactionTs)); + will(returnValue(Futures.immediateFailedFuture(new RuntimeException()))); + never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); + }}; + } + private TransactionConfig transactionConfig; - protected final TimestampCache timestampCache = new DefaultTimestampCache( + private final TimestampCache timestampCache = new DefaultTimestampCache( metricsManager.getRegistry(), () -> AtlasDbConstants.DEFAULT_TIMESTAMP_CACHE_SIZE); - protected final ExecutorService getRangesExecutor = Executors.newFixedThreadPool(8); - protected final int defaultGetRangesConcurrency = 2; - protected final TransactionOutcomeMetrics transactionOutcomeMetrics + private final ExecutorService getRangesExecutor = Executors.newFixedThreadPool(8); + private final int defaultGetRangesConcurrency = 2; + private final TransactionOutcomeMetrics transactionOutcomeMetrics = TransactionOutcomeMetrics.create(metricsManager); private WrappingTestTransactionManager wrappedSerializableTxManager; @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {"sync", (Function) SynchronousDelegate::new}, - {"async", (Function) AsyncDelegate::new} + {SYNC, (Function) SynchronousDelegate::new}, + {ASYNC, (Function) AsyncDelegate::new} }; return Arrays.asList(data); } @@ -310,11 +353,7 @@ public void testLockAfterGet() throws Exception { final long transactionTs = timestampService.getFreshTimestamp(); keyValueService.put(TABLE, ImmutableMap.of(cell, PtBytes.EMPTY_BYTE_ARRAY), startTs); - m.checking(new Expectations() {{ - oneOf(kvMock).get(TABLE, ImmutableMap.of(cell, transactionTs)); - will(throwException(new RuntimeException())); - never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); - }}); + m.checking(expectationsMapping.get(name).apply(kvMock, cell, transactionTs, lockMock)); Transaction snapshot = transactionWrapper.apply(new SnapshotTransaction(metricsManager, kvMock, @@ -1448,8 +1487,8 @@ public Transaction delegate() { public Map get(TableReference tableRef, Set cells) { try { return delegate.getAsync(tableRef, cells).get(); - } catch (Exception e) { - throw new RuntimeException(e.getCause()); + } catch (InterruptedException | ExecutionException e) { + throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); } } } From a7b2e9f19561e1d6aa421a0d7b98cf8a9a56a240 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 16:58:22 +0100 Subject: [PATCH 07/16] Rebase fixes --- .../keyvalue/cassandra/async/ThrowingCqlClientImpl.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java index 9144a30ea0c..1070af0c2a9 100644 --- a/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java +++ b/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/async/ThrowingCqlClientImpl.java @@ -34,9 +34,4 @@ public void close() { public ListenableFuture executeQuery(CqlQuerySpec querySpec) { throw new UnsupportedOperationException("Not configured to use CQL client, check your KVS config file."); } - - @Override - public ListenableFuture execute(Executable executable) { - throw new UnsupportedOperationException("Not configured to use CQL client, check your KVS config file."); - } } From 839571ee6307115334989380f518b6c5267dd6a7 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 17:21:30 +0100 Subject: [PATCH 08/16] Self review fixes --- .../transaction/impl/ReadTransaction.java | 1 - .../transaction/impl/SnapshotTransaction.java | 117 +++++++++--------- .../impl/SnapshotTransactionTest.java | 56 +++++---- 3 files changed, 93 insertions(+), 81 deletions(-) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java index 1d570e92c87..2a9fc0e137f 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/ReadTransaction.java @@ -148,5 +148,4 @@ public ListenableFuture> getAsync(TableReference tableRef, Set checkTableName(tableRef); return delegate().getAsync(tableRef, cells); } - } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index a780a0100e7..f80b0338938 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -22,7 +22,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Optional; import java.util.Set; import java.util.SortedMap; @@ -389,7 +388,7 @@ public SortedMap> getRows(TableReference tableRef, Ite } @Override - public Map>> getRowsColumnRange( + public Map>> getRowsColumnRange( TableReference tableRef, Iterable rows, BatchColumnRangeSelection columnRangeSelection) { @@ -398,7 +397,7 @@ public Map>> getRowsColumnRange( } @Override - public Iterator> getRowsColumnRange(TableReference tableRef, + public Iterator> getRowsColumnRange(TableReference tableRef, Iterable rows, ColumnRangeSelection columnRangeSelection, int batchHint) { @@ -417,8 +416,8 @@ public Iterator> getRowsColumnRange(TableReference tableRef, validatePreCommitRequirementsOnReadIfNecessary(tableRef, getStartTimestamp()); } // else the postFiltered iterator will check for each batch. - Iterator> rawResultsByRow = partitionByRow(rawResults); - Iterator>> postFiltered = Iterators.transform(rawResultsByRow, e -> { + Iterator> rawResultsByRow = partitionByRow(rawResults); + Iterator>> postFiltered = Iterators.transform(rawResultsByRow, e -> { byte[] row = e.getKey(); RowColumnRangeIterator rawIterator = e.getValue(); BatchColumnRangeSelection batchColumnRangeSelection = @@ -430,7 +429,7 @@ public Iterator> getRowsColumnRange(TableReference tableRef, } @Override - public Map>> getRowsColumnRangeIterator(TableReference tableRef, + public Map>> getRowsColumnRangeIterator(TableReference tableRef, Iterable rows, BatchColumnRangeSelection columnRangeSelection) { checkGetPreconditions(tableRef); @@ -440,28 +439,28 @@ public Map>> getRowsColumnRangeIterator(Tab hasReads = true; Map rawResults = keyValueService.getRowsColumnRange(tableRef, rows, columnRangeSelection, getStartTimestamp()); - Map>> postFilteredResults = + Map>> postFilteredResults = Maps.newHashMapWithExpectedSize(rawResults.size()); - for (Entry e : rawResults.entrySet()) { + for (Map.Entry e : rawResults.entrySet()) { byte[] row = e.getKey(); RowColumnRangeIterator rawIterator = e.getValue(); - Iterator> postFilteredIterator = + Iterator> postFilteredIterator = getPostFilteredColumns(tableRef, columnRangeSelection, row, rawIterator); postFilteredResults.put(row, postFilteredIterator); } return postFilteredResults; } - private Iterator> getPostFilteredColumns( + private Iterator> getPostFilteredColumns( TableReference tableRef, BatchColumnRangeSelection batchColumnRangeSelection, byte[] row, RowColumnRangeIterator rawIterator) { - Iterator> postFilterIterator = + Iterator> postFilterIterator = getRowColumnRangePostFiltered(tableRef, row, batchColumnRangeSelection, rawIterator); SortedMap localWrites = getLocalWritesForColumnRange(tableRef, batchColumnRangeSelection, row); - Iterator> localIterator = localWrites.entrySet().iterator(); - Iterator> mergedIterator = + Iterator> localIterator = localWrites.entrySet().iterator(); + Iterator> mergedIterator = IteratorUtils.mergeIterators(localIterator, postFilterIterator, Ordering.from(UnsignedBytes.lexicographicalComparator()) @@ -471,8 +470,8 @@ private Iterator> getPostFilteredColumns( return filterDeletedValues(mergedIterator, tableRef); } - private Iterator> filterDeletedValues( - Iterator> unfiltered, + private Iterator> filterDeletedValues( + Iterator> unfiltered, TableReference tableReference) { Meter emptyValueMeter = getMeter(AtlasDbMetricNames.CellFilterMetrics.EMPTY_VALUE, tableReference); return Iterators.filter(unfiltered, entry -> { @@ -484,22 +483,22 @@ private Iterator> filterDeletedValues( }); } - private Iterator> getRowColumnRangePostFiltered( + private Iterator> getRowColumnRangePostFiltered( TableReference tableRef, byte[] row, BatchColumnRangeSelection columnRangeSelection, RowColumnRangeIterator rawIterator) { ColumnRangeBatchProvider batchProvider = new ColumnRangeBatchProvider( keyValueService, tableRef, row, columnRangeSelection, getStartTimestamp()); - BatchSizeIncreasingIterator> batchIterator = new BatchSizeIncreasingIterator<>( + BatchSizeIncreasingIterator> batchIterator = new BatchSizeIncreasingIterator<>( batchProvider, columnRangeSelection.getBatchHint(), ClosableIterators.wrap(rawIterator)); - Iterator>> postFilteredBatches = - new AbstractIterator>>() { + Iterator>> postFilteredBatches = + new AbstractIterator>>() { @Override - protected Iterator> computeNext() { + protected Iterator> computeNext() { ImmutableMap.Builder rawBuilder = ImmutableMap.builder(); - List> batch = batchIterator.getBatch(); - for (Entry result : batch) { + List> batch = batchIterator.getBatch(); + for (Map.Entry result : batch) { rawBuilder.put(result); } Map raw = rawBuilder.build(); @@ -523,22 +522,22 @@ protected Iterator> computeNext() { * that all columns for a single row are adjacent, so this method will return an {@link Iterator} with exactly one * entry per non-empty row. */ - private Iterator> partitionByRow(RowColumnRangeIterator rawResults) { - PeekingIterator> peekableRawResults = Iterators.peekingIterator(rawResults); - return new AbstractIterator>() { + private Iterator> partitionByRow(RowColumnRangeIterator rawResults) { + PeekingIterator> peekableRawResults = Iterators.peekingIterator(rawResults); + return new AbstractIterator>() { byte[] prevRowName; @Override - protected Entry computeNext() { + protected Map.Entry computeNext() { finishConsumingPreviousRow(peekableRawResults); if (!peekableRawResults.hasNext()) { return endOfData(); } byte[] nextRowName = peekableRawResults.peek().getKey().getRowName(); - Iterator> columnsIterator = - new AbstractIterator>() { + Iterator> columnsIterator = + new AbstractIterator>() { @Override - protected Entry computeNext() { + protected Map.Entry computeNext() { if (!peekableRawResults.hasNext() || !Arrays.equals(peekableRawResults.peek().getKey().getRowName(), nextRowName)) { return endOfData(); @@ -550,7 +549,7 @@ protected Entry computeNext() { return Maps.immutableEntry(nextRowName, new LocalRowColumnRangeIterator(columnsIterator)); } - private void finishConsumingPreviousRow(PeekingIterator> iter) { + private void finishConsumingPreviousRow(PeekingIterator> iter) { int numConsumed = 0; while (iter.hasNext() && Arrays.equals(iter.peek().getKey().getRowName(), prevRowName)) { iter.next(); @@ -610,9 +609,9 @@ private void extractLocalWritesForRow( byte[] row, ColumnSelection columnSelection) { Cell lowCell = Cells.createSmallestCellForRow(row); - Iterator> it = writes.tailMap(lowCell).entrySet().iterator(); + Iterator> it = writes.tailMap(lowCell).entrySet().iterator(); while (it.hasNext()) { - Entry entry = it.next(); + Map.Entry entry = it.next(); Cell cell = entry.getKey(); if (!Arrays.equals(row, cell.getRowName())) { break; @@ -779,7 +778,7 @@ public Iterable>> getRanges(final TableRefer byte[] nextStartRowName = getNextStartRowName( rangeRequest, prePostFilter); - List> mergeIterators = getPostFilteredWithLocalWrites( + List> mergeIterators = getPostFilteredWithLocalWrites( tableRef, postFiltered, rangeRequest, @@ -905,18 +904,18 @@ private boolean isThoroughlySwept(TableReference tableRef) { return sweepStrategyManager.get().get(tableRef) == SweepStrategy.THOROUGH; } - private List> getPostFilteredWithLocalWrites(final TableReference tableRef, + private List> getPostFilteredWithLocalWrites(final TableReference tableRef, final SortedMap postFiltered, final RangeRequest rangeRequest, List> prePostFilter, final byte[] endRowExclusive) { Map prePostFilterCells = Cells.convertRowResultsToCells(prePostFilter); - Collection> postFilteredCells = Collections2.filter( + Collection> postFilteredCells = Collections2.filter( postFiltered.entrySet(), Predicates.compose( Predicates.in(prePostFilterCells.keySet()), MapEntries.getKeyFunction())); - Collection> localWritesInRange = getLocalWritesForRange( + Collection> localWritesInRange = getLocalWritesForRange( tableRef, rangeRequest.getStartInclusive(), endRowExclusive).entrySet(); @@ -1019,13 +1018,13 @@ private Iterator> filterEmptyColumnsFromRows( }); } - private List> mergeInLocalWrites( + private List> mergeInLocalWrites( TableReference tableRef, - Iterator> postFilterIterator, - Iterator> localWritesInRange, + Iterator> postFilterIterator, + Iterator> localWritesInRange, boolean isReverse) { - Ordering> ordering = Ordering.natural().onResultOf(MapEntries.getKeyFunction()); - Iterator> mergeIterators = IteratorUtils.mergeIterators( + Ordering> ordering = Ordering.natural().onResultOf(MapEntries.getKeyFunction()); + Iterator> mergeIterators = IteratorUtils.mergeIterators( postFilterIterator, localWritesInRange, isReverse ? ordering.reverse() : ordering, from -> from.rhSide); // always override their value with written values @@ -1033,15 +1032,15 @@ private List> mergeInLocalWrites( return postFilterEmptyValues(tableRef, mergeIterators); } - private List> postFilterEmptyValues( + private List> postFilterEmptyValues( TableReference tableRef, - Iterator> mergeIterators) { - List> mergedWritesWithoutEmptyValues = new ArrayList<>(); - Predicate> nonEmptyValuePredicate = Predicates.compose(Predicates.not(Value::isTombstone), + Iterator> mergeIterators) { + List> mergedWritesWithoutEmptyValues = new ArrayList<>(); + Predicate> nonEmptyValuePredicate = Predicates.compose(Predicates.not(Value::isTombstone), MapEntries.getValueFunction()); long numEmptyValues = 0; while (mergeIterators.hasNext()) { - Entry next = mergeIterators.next(); + Map.Entry next = mergeIterators.next(); if (nonEmptyValuePredicate.apply(next)) { mergedWritesWithoutEmptyValues.add(next); } else { @@ -1152,7 +1151,7 @@ private SortedMap postFilterRows(TableReference tableRef, Map rawResults = Maps.newHashMapWithExpectedSize(estimateSize(rangeRows)); for (RowResult rowResult : rangeRows) { - for (Entry e : rowResult.getColumns().entrySet()) { + for (Map.Entry e : rowResult.getColumns().entrySet()) { rawResults.put(Cell.create(rowResult.getRowName(), e.getKey()), e.getValue()); } } @@ -1175,7 +1174,7 @@ private void getWithPostFiltering(TableReference tableRef, @Output ImmutableMap.Builder results, Function transformer) { long bytes = 0; - for (Entry e : rawResults.entrySet()) { + for (Map.Entry e : rawResults.entrySet()) { bytes += e.getValue().getContents().length + Cells.getApproxSizeOfCell(e.getKey()); } if (bytes > TransactionConstants.WARN_LEVEL_FOR_QUEUED_BYTES && log.isWarnEnabled()) { @@ -1199,7 +1198,7 @@ private void getWithPostFiltering(TableReference tableRef, // hidden tables are used outside of the transaction protocol, and in general have invalid timestamps, // so do not apply post-filtering as post-filtering would rollback (actually delete) the data incorrectly // this case is hit when reading a hidden table from console - for (Entry e : rawResults.entrySet()) { + for (Map.Entry e : rawResults.entrySet()) { results.put(e.getKey(), transformer.apply(e.getValue())); } return; @@ -1251,7 +1250,7 @@ private Map getWithPostFilteringInternal(TableReference tableRe ImmutableSet.Builder keysAddedBuilder = ImmutableSet.builder(); Set orphanedSentinels = findOrphanedSweepSentinels(tableRef, rawResults); - for (Entry e : rawResults.entrySet()) { + for (Map.Entry e : rawResults.entrySet()) { Cell key = e.getKey(); Value value = e.getValue(); @@ -1369,7 +1368,7 @@ public void putInternal(TableReference tableRef, Map values) { } private void ensureNoEmptyValues(Map values) { - for (Entry cellEntry : values.entrySet()) { + for (Map.Entry cellEntry : values.entrySet()) { if ((cellEntry.getValue() == null) || (cellEntry.getValue().length == 0)) { throw new SafeIllegalArgumentException( "AtlasDB does not currently support inserting null or empty (zero-byte) values."); @@ -1378,7 +1377,7 @@ private void ensureNoEmptyValues(Map values) { } private void putWritesAndLogIfTooLarge(Map values, SortedMap writes) { - for (Entry e : values.entrySet()) { + for (Map.Entry e : values.entrySet()) { byte[] val = MoreObjects.firstNonNull(e.getValue(), PtBytes.EMPTY_BYTE_ARRAY); Cell cell = e.getKey(); if (writes.put(cell, val) == null) { @@ -1493,7 +1492,7 @@ public void commit(TransactionService transactionService) { private void checkConstraints() { List violations = Lists.newArrayList(); - for (Entry entry : constraintsByTableName.entrySet()) { + for (Map.Entry entry : constraintsByTableName.entrySet()) { SortedMap sortedMap = writesByTable.get(entry.getKey()); if (sortedMap != null) { violations.addAll(entry.getValue().findConstraintFailures(sortedMap, this, constraintCheckingMode)); @@ -1672,7 +1671,7 @@ private Set refreshCommitAndImmutableTsLocks(@Nullable LockToken comm */ protected void throwIfConflictOnCommit(LockToken commitLocksToken, TransactionService transactionService) throws TransactionConflictException { - for (Entry> write : writesByTable.entrySet()) { + for (Map.Entry> write : writesByTable.entrySet()) { ConflictHandler conflictHandler = getConflictHandlerForTable(write.getKey()); throwIfWriteAlreadyCommitted( write.getKey(), @@ -1736,7 +1735,7 @@ private void throwIfValueChangedConflict(TableReference table, Map conflictingValues = keyValueService.get(table, cellToTs); Set conflictingCells = Sets.newHashSet(); - for (Entry cellEntry : cellToTs.entrySet()) { + for (Map.Entry cellEntry : cellToTs.entrySet()) { Cell cell = cellEntry.getKey(); if (!writes.containsKey(cell)) { Validate.isTrue(false, "Missing write for cell: %s for table %s", cellToConflict.get(cell), table); @@ -1794,7 +1793,7 @@ protected Map detectWriteAlreadyCommittedInternal(TableReference tab Map commitTimestamps = getCommitTimestamps(tableRef, rawResults.values(), false); Map keysToDelete = Maps.newHashMapWithExpectedSize(0); - for (Entry e : rawResults.entrySet()) { + for (Map.Entry e : rawResults.entrySet()) { Cell key = e.getKey(); long theirStartTimestamp = e.getValue(); AssertUtils.assertAndLog(log, theirStartTimestamp != getStartTimestamp(), @@ -2082,7 +2081,7 @@ protected Map getCommitTimestamps(@Nullable TableReference tableRef, Map rawResults = loadCommitTimestamps(gets); - for (Entry e : rawResults.entrySet()) { + for (Map.Entry e : rawResults.entrySet()) { if (e.getValue() != null) { long startTs = e.getKey(); long commitTs = e.getValue(); @@ -2212,7 +2211,7 @@ private Multimap getCellsToScrubByCell(State expectedState Multimap cellToTableName = HashMultimap.create(); State actualState = state.get(); if (expectedState == actualState) { - for (Entry> entry : writesByTable.entrySet()) { + for (Map.Entry> entry : writesByTable.entrySet()) { TableReference table = entry.getKey(); Set cells = entry.getValue().keySet(); for (Cell c : cells) { @@ -2230,7 +2229,7 @@ private Multimap getCellsToScrubByTable(State expectedStat Multimap tableRefToCells = HashMultimap.create(); State actualState = state.get(); if (expectedState == actualState) { - for (Entry> entry : writesByTable.entrySet()) { + for (Map.Entry> entry : writesByTable.entrySet()) { TableReference table = entry.getKey(); Set cells = entry.getValue().keySet(); tableRefToCells.putAll(table, cells); diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index d6e3d265a7f..72019137e95 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -753,7 +753,8 @@ private List>> getThoroug tasks.add(Pair.of("getRowsColumnRangeIterator", (t, heldLocks) -> { Collection>> results = - t.getRowsColumnRangeIterator(TABLE_SWEPT_THOROUGH, + t.getRowsColumnRangeIterator( + TABLE_SWEPT_THOROUGH, Collections.singleton(PtBytes.toBytes("row1")), BatchColumnRangeSelection.create(new ColumnRangeSelection(null, null), batchHint)) .values(); @@ -901,16 +902,19 @@ public void noRetryOnExpiredLockTokens() throws InterruptedException { @Test public void commitIfPreCommitConditionSucceeds() { - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(PreCommitConditions.NO_OP, (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict( + PreCommitConditions.NO_OP, + (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); } @Test public void failToCommitIfPreCommitConditionFails() { try { - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(ALWAYS_FAILS_CONDITION, + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict( + ALWAYS_FAILS_CONDITION, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; @@ -927,10 +931,12 @@ public void commitWithPreCommitConditionOnRetry() { when(conditionSupplier.get()).thenReturn(ALWAYS_FAILS_CONDITION) .thenReturn(PreCommitConditions.NO_OP); - wrappedSerializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + wrappedSerializableTxManager.runTaskWithConditionWithRetry( + conditionSupplier, + (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); } @Test @@ -981,10 +987,12 @@ public void cleanup() {} }; Supplier conditionSupplier = Suppliers.ofInstance(nonRetriableFailure); try { - wrappedSerializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + wrappedSerializableTxManager.runTaskWithConditionWithRetry( + conditionSupplier, + (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); fail(); } catch (TransactionFailedNonRetriableException e) { assertThat(e.getMessage(), containsString("Condition failed")); @@ -993,7 +1001,8 @@ public void cleanup() {} @Test public void readTransactionSucceedsIfConditionSucceeds() { - wrappedSerializableTxManager.runTaskWithConditionReadOnly(PreCommitConditions.NO_OP, + wrappedSerializableTxManager.runTaskWithConditionReadOnly( + PreCommitConditions.NO_OP, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); TransactionOutcomeMetricsAssert.assertThat(transactionOutcomeMetrics) .hasSuccessfulCommits(1); @@ -1002,7 +1011,8 @@ public void readTransactionSucceedsIfConditionSucceeds() { @Test public void readTransactionFailsIfConditionFails() { try { - wrappedSerializableTxManager.runTaskWithConditionReadOnly(ALWAYS_FAILS_CONDITION, + wrappedSerializableTxManager.runTaskWithConditionReadOnly( + ALWAYS_FAILS_CONDITION, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); fail(); } catch (TransactionFailedRetriableException e) { @@ -1025,10 +1035,12 @@ public void cleanup() { } }; - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(succeedsCondition, (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict( + succeedsCondition, + (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); assertThat(counter.intValue(), is(1)); wrappedSerializableTxManager.runTaskWithConditionReadOnly(succeedsCondition, @@ -1387,7 +1399,9 @@ private void writeCells(TableReference table, ImmutableMap cellsTo private RowResult readRow(byte[] defaultRow) { Transaction readTransaction = txManager.createNewTransaction(); - SortedMap> allRows = readTransaction.getRows(TABLE, ImmutableSet.of(defaultRow), + SortedMap> allRows = readTransaction.getRows( + TABLE, + ImmutableSet.of(defaultRow), ColumnSelection.all()); return allRows.get(defaultRow); } From 565106115d53b5781d44e2658c2ad983058e36a8 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 17:26:40 +0100 Subject: [PATCH 09/16] Using Throwables. --- .../CassandraKeyValueServiceTransactionIntegrationTest.java | 5 +++-- .../cassandra/CassandraKvsSerializableTransactionTest.java | 5 +++-- .../atlasdb/transaction/impl/CachingTransactionTest.java | 5 +++-- .../atlasdb/transaction/impl/SnapshotTransactionTest.java | 1 - 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java index 9201a800098..3c8fbc981f1 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -174,8 +175,8 @@ public Transaction delegate() { public Map get(TableReference tableRef, Set cells) { try { return delegate.getAsync(tableRef, cells).get(); - } catch (Exception e) { - throw new RuntimeException(e.getCause()); + } catch (InterruptedException | ExecutionException e) { + throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); } } } diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java index a8e8378ed38..76b6db068e2 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java @@ -21,6 +21,7 @@ import java.util.Collection; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.function.Function; import org.junit.ClassRule; @@ -120,8 +121,8 @@ public Transaction delegate() { public Map get(TableReference tableRef, Set cells) { try { return delegate.getAsync(tableRef, cells).get(); - } catch (Exception e) { - throw new RuntimeException(e.getCause()); + } catch (InterruptedException | ExecutionException e) { + throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); } } } diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java index aa222429112..11159859c35 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java @@ -20,6 +20,7 @@ import java.util.Map; import java.util.Set; import java.util.SortedMap; +import java.util.concurrent.ExecutionException; import java.util.function.BiFunction; import java.util.function.Function; @@ -209,8 +210,8 @@ private static class AsyncDelegate extends CachingTransaction { public Map get(TableReference tableRef, Set cells) { try { return super.getAsync(tableRef, cells).get(); - } catch (Exception e) { - throw new RuntimeException(e.getCause()); + } catch (InterruptedException | ExecutionException e) { + throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); } } } diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index 72019137e95..9d662ff71b9 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -287,7 +287,6 @@ public void setUp() throws Exception { txManager = new WrappingTestTransactionManager(txManager, transactionWrapper); wrappedSerializableTxManager = new WrappingTestTransactionManager(serializableTxManager, transactionWrapper); - transactionConfig = ImmutableTransactionConfig.builder().build(); keyValueService.createTable(TABLE, AtlasDbConstants.GENERIC_TABLE_METADATA); From 4ff037cec5584cb1368f3e344ac71f8d14c7fc02 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 16:26:40 +0000 Subject: [PATCH 10/16] Add generated changelog entries --- changelog/@unreleased/pr-4301.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-4301.v2.yml diff --git a/changelog/@unreleased/pr-4301.v2.yml b/changelog/@unreleased/pr-4301.v2.yml new file mode 100644 index 00000000000..948664eb2d1 --- /dev/null +++ b/changelog/@unreleased/pr-4301.v2.yml @@ -0,0 +1,6 @@ +type: feature +feature: + description: | + Transactions can now support asynchronous get operations. This has been implemented for all transaction types. Feature is designed for direct uses of transactions. + links: + - https://github.com/palantir/atlasdb/pull/4301 From 7bd30a9744b92746d642078d9a0ef3a2095f014c Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Mon, 14 Oct 2019 17:50:09 +0100 Subject: [PATCH 11/16] Style fix --- .../palantir/atlasdb/transaction/impl/SnapshotTransaction.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index f80b0338938..3cab8539e31 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -1036,7 +1036,8 @@ private List> postFilterEmptyValues( TableReference tableRef, Iterator> mergeIterators) { List> mergedWritesWithoutEmptyValues = new ArrayList<>(); - Predicate> nonEmptyValuePredicate = Predicates.compose(Predicates.not(Value::isTombstone), + Predicate> nonEmptyValuePredicate = Predicates.compose( + Predicates.not(Value::isTombstone), MapEntries.getValueFunction()); long numEmptyValues = 0; while (mergeIterators.hasNext()) { From 299bb13607213544a67351b8bb6d7af3509eeef5 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Tue, 15 Oct 2019 10:40:40 +0100 Subject: [PATCH 12/16] Snapshot get async refactor. --- .../transaction/impl/CachingTransaction.java | 36 ++------------ .../impl/SerializableTransaction.java | 16 +++--- .../transaction/impl/SnapshotTransaction.java | 49 +++---------------- 3 files changed, 18 insertions(+), 83 deletions(-) diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java index 1004e655dd3..7bf80b6db20 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java @@ -128,6 +128,11 @@ public Map get(TableReference tableRef, Set cells) { } } + @Override + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + return get(tableRef, cells, (super::getAsync)); + } + private ListenableFuture> get(TableReference tableRef, Set cells, CellLoader cellLoader) { if (cells.isEmpty()) { return Futures.immediateFuture(ImmutableMap.of()); @@ -155,37 +160,6 @@ private ListenableFuture> get(TableReference tableRef, Set> getAsync(TableReference tableRef, Set cells) { - if (cells.isEmpty()) { - return Futures.immediateFuture(ImmutableMap.of()); - } - - Set toLoad = Sets.newHashSet(); - ImmutableMap.Builder cacheHitMapBuilder = ImmutableMap.builder(); - for (Cell cell : cells) { - byte[] val = getCachedCellIfPresent(tableRef, cell); - if (val != null) { - if (val.length > 0) { - cacheHitMapBuilder.put(cell, val); - } - } else { - toLoad.add(cell); - } - } - - ImmutableMap cacheHit = cacheHitMapBuilder.build(); - - return Futures.transform(super.getAsync(tableRef, toLoad), - loaded -> { - cacheLoadedCells(tableRef, toLoad, loaded); - return ImmutableMap.builder() - .putAll(loaded.entrySet()) - .putAll(cacheHit.entrySet()) - .build(); - }, MoreExecutors.directExecutor()); - } - @Override public final void delete(TableReference tableRef, Set cells) { super.delete(tableRef, cells); diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java index 7462b8d03d8..b051b4f245a 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java @@ -262,6 +262,12 @@ public Map get(TableReference tableRef, Set cells) { } } + @Override + @Idempotent + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + return get(tableRef, cells, (tableReference, toRead) -> super.getAsync(tableRef, toRead)); + } + public ListenableFuture> get(TableReference tableRef, Set cells, CellLoader cellLoader) { return Futures.transform(cellLoader.load(tableRef, cells), loadedCells -> { @@ -276,16 +282,6 @@ private interface CellLoader { ListenableFuture> load(TableReference tableReference, Set toRead); } - @Override - public ListenableFuture> getAsync(TableReference tableRef, Set cells) { - return Futures.transform(super.getAsync(tableRef, cells), - ret -> { - markCellsRead(tableRef, cells, ret); - return ret; - }, - MoreExecutors.directExecutor()); - } - @Override @Idempotent public BatchingVisitable> getRange(TableReference tableRef, RangeRequest rangeRequest) { diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index 3cab8539e31..619c41ee3c6 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -119,6 +119,7 @@ import com.palantir.atlasdb.transaction.impl.metrics.TransactionOutcomeMetrics; import com.palantir.atlasdb.transaction.service.TransactionService; import com.palantir.atlasdb.util.MetricsManager; +import com.palantir.common.annotation.Idempotent; import com.palantir.common.annotation.Output; import com.palantir.common.base.AbortingVisitor; import com.palantir.common.base.AbstractBatchingVisitable; @@ -313,48 +314,6 @@ public void disableReadWriteConflictChecking(TableReference tableRef) { conflictDetectionManager.disableReadWriteConflict(tableRef); } - @Override - public ListenableFuture> getAsync(TableReference tableRef, Set cells) { - Timer.Context timer = getTimer("get").time(); - checkGetPreconditions(tableRef); - if (Iterables.isEmpty(cells)) { - return Futures.immediateFuture(ImmutableMap.of()); - } - hasReads = true; - - ImmutableMap.Builder resultBuilder = ImmutableMap.builder(); - SortedMap writes = writesByTable.get(tableRef); - if (writes != null) { - for (Cell cell : cells) { - if (writes.containsKey(cell)) { - resultBuilder.put(cell, writes.get(cell)); - } - } - } - - Map writtenLocally = resultBuilder.build(); - - return Futures.transform( - // We don't need to read any cells that were written locally. - getFromKeyValueServiceAsync(tableRef, Sets.difference(cells, writtenLocally.keySet())), - keyValueResult -> { - ImmutableMap result = ImmutableMap.builder() - .putAll(writtenLocally) - .putAll(keyValueResult) - .build(); - - long getMillis = TimeUnit.NANOSECONDS.toMillis(timer.stop()); - if (perfLogger.isDebugEnabled()) { - perfLogger.debug("get({}, {} cells) found {} cells (some possibly deleted), took {} ms", - tableRef, cells.size(), result.size(), getMillis); - } - validatePreCommitRequirementsOnReadIfNecessary(tableRef, getStartTimestamp()); - return removeEmptyColumns(result, tableRef); - }, - MoreExecutors.directExecutor()); - } - - @Override public SortedMap> getRows(TableReference tableRef, Iterable rows, ColumnSelection columnSelection) { @@ -637,6 +596,12 @@ public Map get(TableReference tableRef, Set cells) { } } + @Override + @Idempotent + public ListenableFuture> getAsync(TableReference tableRef, Set cells) { + return get("getAsync", tableRef, cells, keyValueService::getAsync); + } + private ListenableFuture> get( String operationName, TableReference tableRef, From c224fc3ddea060ec5d0ce6d480ea17ef850f96fc Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Tue, 15 Oct 2019 15:57:36 +0100 Subject: [PATCH 13/16] Extracted delegates. --- ...alueServiceTransactionIntegrationTest.java | 52 ++------------ ...ssandraKvsSerializableTransactionTest.java | 52 ++------------ .../transaction/impl/CachingTransaction.java | 9 ++- .../impl/SerializableTransaction.java | 8 ++- .../transaction/impl/SnapshotTransaction.java | 7 +- .../transaction/impl/GetAsyncDelegate.java | 49 +++++++++++++ .../impl/GetSynchronousDelegate.java | 43 ++++++++++++ .../impl/CachingTransactionTest.java | 68 ++++++------------- .../impl/SnapshotTransactionTest.java | 45 +----------- 9 files changed, 137 insertions(+), 196 deletions(-) create mode 100644 atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java create mode 100644 atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetSynchronousDelegate.java diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java index 3c8fbc981f1..4b8c2a557c2 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java @@ -18,9 +18,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -40,13 +37,12 @@ import com.google.common.base.Suppliers; import com.google.common.util.concurrent.Futures; import com.palantir.atlasdb.containers.CassandraResource; -import com.palantir.atlasdb.keyvalue.api.Cell; import com.palantir.atlasdb.keyvalue.api.KeyAlreadyExistsException; import com.palantir.atlasdb.keyvalue.api.KeyValueService; -import com.palantir.atlasdb.keyvalue.api.TableReference; import com.palantir.atlasdb.transaction.api.Transaction; import com.palantir.atlasdb.transaction.impl.AbstractTransactionTest; -import com.palantir.atlasdb.transaction.impl.ForwardingTransaction; +import com.palantir.atlasdb.transaction.impl.GetAsyncDelegate; +import com.palantir.atlasdb.transaction.impl.GetSynchronousDelegate; import com.palantir.atlasdb.transaction.impl.TransactionTables; import com.palantir.atlasdb.transaction.service.SimpleTransactionService; import com.palantir.atlasdb.transaction.service.TransactionService; @@ -66,8 +62,8 @@ public class CassandraKeyValueServiceTransactionIntegrationTest extends Abstract @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {SYNC, (Function) SynchronousDelegate::new}, - {ASYNC, (Function) AsyncDelegate::new} + {SYNC, (Function) GetSynchronousDelegate::new}, + {ASYNC, (Function) GetAsyncDelegate::new} }; return Arrays.asList(data); } @@ -140,44 +136,4 @@ protected boolean supportsReverse() { protected Transaction startTransaction() { return transactionWrapper.apply(super.startTransaction()); } - - private static class SynchronousDelegate extends ForwardingTransaction { - private final Transaction delegate; - - SynchronousDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - return delegate.get(tableRef, cells); - } - } - - private static class AsyncDelegate extends ForwardingTransaction { - private final Transaction delegate; - - AsyncDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - try { - return delegate.getAsync(tableRef, cells).get(); - } catch (InterruptedException | ExecutionException e) { - throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); - } - } - } } diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java index 76b6db068e2..95a46833e9c 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java @@ -19,9 +19,6 @@ import java.util.Arrays; import java.util.Collection; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.ExecutionException; import java.util.function.Function; import org.junit.ClassRule; @@ -29,15 +26,14 @@ import org.junit.runners.Parameterized; import com.palantir.atlasdb.containers.CassandraResource; -import com.palantir.atlasdb.keyvalue.api.Cell; -import com.palantir.atlasdb.keyvalue.api.TableReference; import com.palantir.atlasdb.sweep.metrics.TargetedSweepMetrics; import com.palantir.atlasdb.sweep.queue.MultiTableSweepQueueWriter; import com.palantir.atlasdb.sweep.queue.SweepQueue; import com.palantir.atlasdb.sweep.queue.TargetedSweeper; import com.palantir.atlasdb.transaction.api.Transaction; import com.palantir.atlasdb.transaction.impl.AbstractSerializableTransactionTest; -import com.palantir.atlasdb.transaction.impl.ForwardingTransaction; +import com.palantir.atlasdb.transaction.impl.GetAsyncDelegate; +import com.palantir.atlasdb.transaction.impl.GetSynchronousDelegate; @RunWith(Parameterized.class) public class CassandraKvsSerializableTransactionTest extends AbstractSerializableTransactionTest { @@ -47,8 +43,8 @@ public class CassandraKvsSerializableTransactionTest extends AbstractSerializabl @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {"sync", (Function) SynchronousDelegate::new}, - {"async", (Function) AsyncDelegate::new} + {"sync", (Function) GetSynchronousDelegate::new}, + {"async", (Function) GetAsyncDelegate::new} }; return Arrays.asList(data); } @@ -86,44 +82,4 @@ protected MultiTableSweepQueueWriter getSweepQueueWriterInitialized() { protected boolean supportsReverse() { return false; } - - private static class SynchronousDelegate extends ForwardingTransaction { - private final Transaction delegate; - - SynchronousDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - return delegate.get(tableRef, cells); - } - } - - private static class AsyncDelegate extends ForwardingTransaction { - private final Transaction delegate; - - AsyncDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - try { - return delegate.getAsync(tableRef, cells).get(); - } catch (InterruptedException | ExecutionException e) { - throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); - } - } - } } diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java index 7bf80b6db20..80a1f7849fd 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/transaction/impl/CachingTransaction.java @@ -119,7 +119,7 @@ public SortedMap> getRows(TableReference tableRef, Ite @Override public Map get(TableReference tableRef, Set cells) { try { - return get( + return getWithLoader( tableRef, cells, (tableReference, toRead) -> Futures.immediateFuture(super.get(tableReference, toRead))).get(); @@ -130,10 +130,13 @@ public Map get(TableReference tableRef, Set cells) { @Override public ListenableFuture> getAsync(TableReference tableRef, Set cells) { - return get(tableRef, cells, (super::getAsync)); + return getWithLoader(tableRef, cells, super::getAsync); } - private ListenableFuture> get(TableReference tableRef, Set cells, CellLoader cellLoader) { + private ListenableFuture> getWithLoader( + TableReference tableRef, + Set cells, + CellLoader cellLoader) { if (cells.isEmpty()) { return Futures.immediateFuture(ImmutableMap.of()); } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java index b051b4f245a..6045419693d 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java @@ -253,7 +253,7 @@ public Entry next() { @Idempotent public Map get(TableReference tableRef, Set cells) { try { - return get( + return getWithLoader( tableRef, cells, (tableReference, toRead) -> Futures.immediateFuture(super.get(tableRef, toRead))).get(); @@ -265,10 +265,12 @@ public Map get(TableReference tableRef, Set cells) { @Override @Idempotent public ListenableFuture> getAsync(TableReference tableRef, Set cells) { - return get(tableRef, cells, (tableReference, toRead) -> super.getAsync(tableRef, toRead)); + return getWithLoader(tableRef, cells, super::getAsync); } - public ListenableFuture> get(TableReference tableRef, Set cells, CellLoader cellLoader) { + private ListenableFuture> getWithLoader( + TableReference tableRef, Set cells, + CellLoader cellLoader) { return Futures.transform(cellLoader.load(tableRef, cells), loadedCells -> { markCellsRead(tableRef, cells, loadedCells); diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index 619c41ee3c6..0860ff18f19 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -583,9 +583,10 @@ private void extractLocalWritesForRow( } @Override + @Idempotent public Map get(TableReference tableRef, Set cells) { try { - return get( + return getWithLoader( "get", tableRef, cells, @@ -599,10 +600,10 @@ public Map get(TableReference tableRef, Set cells) { @Override @Idempotent public ListenableFuture> getAsync(TableReference tableRef, Set cells) { - return get("getAsync", tableRef, cells, keyValueService::getAsync); + return getWithLoader("getAsync", tableRef, cells, keyValueService::getAsync); } - private ListenableFuture> get( + private ListenableFuture> getWithLoader( String operationName, TableReference tableRef, Set cells, diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java new file mode 100644 index 00000000000..69c955d375a --- /dev/null +++ b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java @@ -0,0 +1,49 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.atlasdb.transaction.impl; + +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ExecutionException; + +import com.palantir.atlasdb.keyvalue.api.Cell; +import com.palantir.atlasdb.keyvalue.api.TableReference; +import com.palantir.atlasdb.transaction.api.Transaction; +import com.palantir.common.base.Throwables; + +public class GetAsyncDelegate extends ForwardingTransaction { + + private final Transaction delegate; + + public GetAsyncDelegate(Transaction transaction) { + this.delegate = transaction; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + try { + return super.getAsync(tableRef, cells).get(); + } catch (InterruptedException | ExecutionException e) { + throw Throwables.rewrapAndThrowUncheckedException(e.getCause()); + } + } +} diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetSynchronousDelegate.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetSynchronousDelegate.java new file mode 100644 index 00000000000..61e86b0de1b --- /dev/null +++ b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetSynchronousDelegate.java @@ -0,0 +1,43 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.atlasdb.transaction.impl; + +import java.util.Map; +import java.util.Set; + +import com.palantir.atlasdb.keyvalue.api.Cell; +import com.palantir.atlasdb.keyvalue.api.TableReference; +import com.palantir.atlasdb.transaction.api.Transaction; + +public class GetSynchronousDelegate extends ForwardingTransaction { + + private final Transaction delegate; + + public GetSynchronousDelegate(Transaction delegate) { + this.delegate = delegate; + } + + @Override + public Transaction delegate() { + return delegate; + } + + @Override + public Map get(TableReference tableRef, Set cells) { + return super.get(tableRef, cells); + } +} diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java index 11159859c35..31cee914369 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java @@ -20,7 +20,6 @@ import java.util.Map; import java.util.Set; import java.util.SortedMap; -import java.util.concurrent.ExecutionException; import java.util.function.BiFunction; import java.util.function.Function; @@ -55,16 +54,16 @@ public class CachingTransactionTest { @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {SYNC, (Function) SynchronousDelegate::new}, - {ASYNC, (Function) AsyncDelegate::new} + {SYNC, (Function) GetSynchronousDelegate::new}, + {ASYNC, (Function) GetAsyncDelegate::new} }; return Arrays.asList(data); } private final TableReference table = TableReference.createWithEmptyNamespace("table"); private final Mockery mockery = new Mockery(); - private final Transaction txn = mockery.mock(Transaction.class); - private final CachingTransaction ct; + private final Transaction transaction = mockery.mock(Transaction.class); + private final Transaction cachingTransaction; private final String name; private final Map, Map, Expectations>> expectationsMapping = ImmutableMap., Map, Expectations>>builder() @@ -72,9 +71,9 @@ public static Collection data() { .put(ASYNC, CachingTransactionTest.this::asyncGetExpectation) .build(); - public CachingTransactionTest(String name, Function transactionWrapper) { + public CachingTransactionTest(String name, Function transactionWrapper) { this.name = name; - ct = transactionWrapper.apply(new CachingTransaction(txn)); + cachingTransaction = transactionWrapper.apply(new CachingTransaction(transaction)); } @Test @@ -88,16 +87,16 @@ public void testCacheEmptyGets() { mockery.checking(new Expectations() { { - oneOf(txn).getRows(table, oneRow, oneColumn); + oneOf(transaction).getRows(table, oneRow, oneColumn); will(returnValue(emptyResults)); - oneOf(txn).getRows(table, noRows, oneColumn); + oneOf(transaction).getRows(table, noRows, oneColumn); will(returnValue(emptyResults)); } }); - Assert.assertEquals(emptyResults, ct.getRows(table, oneRow, oneColumn)); - Assert.assertEquals(emptyResults, ct.getRows(table, oneRow, oneColumn)); + Assert.assertEquals(emptyResults, cachingTransaction.getRows(table, oneRow, oneColumn)); + Assert.assertEquals(emptyResults, cachingTransaction.getRows(table, oneRow, oneColumn)); mockery.assertIsSatisfied(); } @@ -120,16 +119,16 @@ public void testGetRows() { mockery.checking(new Expectations() { { // row result is cached after first call, so second call requests no rows - oneOf(txn).getRows(table, oneRow, oneColumn); + oneOf(transaction).getRows(table, oneRow, oneColumn); will(returnValue(oneResult)); - oneOf(txn).getRows(table, noRows, oneColumn); + oneOf(transaction).getRows(table, noRows, oneColumn); will(returnValue(emptyResults)); } }); - Assert.assertEquals(oneResult, ct.getRows(table, oneRow, oneColumn)); - Assert.assertEquals(oneResult, ct.getRows(table, oneRow, oneColumn)); + Assert.assertEquals(oneResult, cachingTransaction.getRows(table, oneRow, oneColumn)); + Assert.assertEquals(oneResult, cachingTransaction.getRows(table, oneRow, oneColumn)); mockery.assertIsSatisfied(); } @@ -158,8 +157,8 @@ private void testGetCellResults(Cell cell, Map cellValueMap) { final Set cellSet = ImmutableSet.of(cell); mockery.checking(expectationsMapping.get(name).apply(cellSet, cellValueMap)); - Assert.assertEquals(cellValueMap, ct.get(table, cellSet)); - Assert.assertEquals(cellValueMap, ct.get(table, cellSet)); + Assert.assertEquals(cellValueMap, cachingTransaction.get(table, cellSet)); + Assert.assertEquals(cellValueMap, cachingTransaction.get(table, cellSet)); mockery.assertIsSatisfied(); } @@ -167,10 +166,10 @@ private void testGetCellResults(Cell cell, Map cellValueMap) { private Expectations syncGetExpectation(Set cellSet, Map cellValueMap) { return new Expectations() { { - oneOf(txn).get(table, cellSet); + oneOf(transaction).get(table, cellSet); will(returnValue(cellValueMap)); - oneOf(txn).get(table, ImmutableSet.of()); + oneOf(transaction).get(table, ImmutableSet.of()); will(returnValue(ImmutableMap.of())); } }; @@ -179,40 +178,13 @@ private Expectations syncGetExpectation(Set cellSet, Map cel private Expectations asyncGetExpectation(Set cellSet, Map cellValueMap) { return new Expectations() { { - oneOf(txn).getAsync(table, cellSet); + oneOf(transaction).getAsync(table, cellSet); will(returnValue(Futures.immediateFuture(cellValueMap))); - oneOf(txn).getAsync(table, ImmutableSet.of()); + oneOf(transaction).getAsync(table, ImmutableSet.of()); will(returnValue(Futures.immediateFuture(ImmutableMap.of()))); } }; } - private static class SynchronousDelegate extends CachingTransaction { - - SynchronousDelegate(CachingTransaction transaction) { - super(transaction.delegate()); - } - - @Override - public Map get(TableReference tableRef, Set cells) { - return super.get(tableRef, cells); - } - } - - private static class AsyncDelegate extends CachingTransaction { - - AsyncDelegate(CachingTransaction transaction) { - super(transaction.delegate()); - } - - @Override - public Map get(TableReference tableRef, Set cells) { - try { - return super.getAsync(tableRef, cells).get(); - } catch (InterruptedException | ExecutionException e) { - throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); - } - } - } } diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index 9d662ff71b9..adf658b18d4 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -45,7 +45,6 @@ import java.util.Map; import java.util.Optional; import java.util.Random; -import java.util.Set; import java.util.SortedMap; import java.util.concurrent.Callable; import java.util.concurrent.CompletionService; @@ -207,8 +206,8 @@ private Expectations asyncGetExpectation( @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {SYNC, (Function) SynchronousDelegate::new}, - {ASYNC, (Function) AsyncDelegate::new} + {SYNC, (Function) GetSynchronousDelegate::new}, + {ASYNC, (Function) GetAsyncDelegate::new} }; return Arrays.asList(data); } @@ -1466,46 +1465,6 @@ private static SnapshotTransaction unwrapSnapshotTransaction(Transaction caching return (SnapshotTransaction) unwrapped; } - private static class SynchronousDelegate extends ForwardingTransaction { - private final Transaction delegate; - - SynchronousDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - return delegate.get(tableRef, cells); - } - } - - private static class AsyncDelegate extends ForwardingTransaction { - private final Transaction delegate; - - AsyncDelegate(Transaction transaction) { - this.delegate = transaction; - } - - @Override - public Transaction delegate() { - return delegate; - } - - @Override - public Map get(TableReference tableRef, Set cells) { - try { - return delegate.getAsync(tableRef, cells).get(); - } catch (InterruptedException | ExecutionException e) { - throw com.palantir.common.base.Throwables.rewrapAndThrowUncheckedException(e.getCause()); - } - } - } - private static class WrappingTestTransactionManager implements TestTransactionManager, AutoDelegate_TransactionManager { From 2e9c61b7278f0ae5a382fcc57ee52ea93473b03d Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Tue, 15 Oct 2019 17:17:11 +0100 Subject: [PATCH 14/16] Remove unused. --- .../transaction/impl/SnapshotTransaction.java | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java index 0860ff18f19..1f6a42be8dd 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java @@ -686,23 +686,6 @@ private interface CellLoader { ListenableFuture> load(TableReference tableReference, Map toRead); } - /** - * This will load the given keys from the underlying key value service and apply postFiltering - * so we have snapshot isolation. If the value in the key value service is the empty array - * this will be included here and needs to be filtered out. - */ - private ListenableFuture> getFromKeyValueServiceAsync(TableReference tableRef, Set cells) { - ImmutableMap.Builder result = ImmutableMap.builderWithExpectedSize(cells.size()); - Map toRead = Cells.constantValueMap(cells, getStartTimestamp()); - return Futures.transform( - keyValueService.getAsync(tableRef, toRead), - rawResults -> { - getWithPostFiltering(tableRef, rawResults, result, Value.GET_VALUE); - return result.build(); - }, - MoreExecutors.directExecutor()); - } - private static byte[] getNextStartRowName( RangeRequest range, TokenBackedBasicResultsPage, byte[]> prePostFilter) { From 0b1374db77cabb51a4b3991c136f33583a752df4 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Wed, 16 Oct 2019 10:42:59 +0100 Subject: [PATCH 15/16] Refactorings based on comments. --- .../atlasdb/transaction/api/Transaction.java | 14 +- ...ssandraKeyValueServiceIntegrationTest.java | 17 +- ...alueServiceTransactionIntegrationTest.java | 10 +- ...ssandraKvsSerializableTransactionTest.java | 12 +- .../atlasdb/logging/KvsProfilingLogger.java | 4 +- .../transaction/impl/GetAsyncDelegate.java | 4 +- .../impl/TestTransactionManager.java | 1 + .../impl/TestTransactionManagerImpl.java | 1 + .../impl/WrappingTestTransactionManager.java | 5 + .../com/palantir/atlasdb/AtlasDbTestCase.java | 11 +- .../impl/CachingTransactionTest.java | 21 ++- .../impl/SnapshotTransactionTest.java | 158 ++++++++---------- changelog/@unreleased/pr-4301.v2.yml | 2 +- 13 files changed, 138 insertions(+), 122 deletions(-) diff --git a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java index 8d1dea3cc59..14e0b8a577e 100644 --- a/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java +++ b/atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java @@ -69,6 +69,17 @@ Map>> getRowsColumnRangeIterator( @Idempotent Map get(TableReference tableRef, Set cells); + /** + * Gets the values associated for each {@code cells} from table specified by {@code tableRef}. It is not guaranteed + * that the actual implementations are in fact asynchronous. + * + * @param tableRef the table from which to get the values + * @param cells the cells for which we want to get the values + * @return a {@link Map} from {@link Cell} to {@code byte[]} representing cell/value pairs + */ + @Idempotent + ListenableFuture> getAsync(TableReference tableRef, Set cells); + /** * Creates a visitable that scans the provided range. * @@ -239,7 +250,4 @@ enum TransactionType { default void disableReadWriteConflictChecking(TableReference tableRef) { throw new UnsupportedOperationException(); } - - @Idempotent - ListenableFuture> getAsync(TableReference tableRef, Set cells); } diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceIntegrationTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceIntegrationTest.java index bb35aa4abc2..e4ad4040a6d 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceIntegrationTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceIntegrationTest.java @@ -38,6 +38,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.function.Function; import java.util.stream.Collectors; @@ -80,6 +81,7 @@ import com.palantir.atlasdb.transaction.api.ConflictHandler; import com.palantir.atlasdb.util.MetricsManager; import com.palantir.atlasdb.util.MetricsManagers; +import com.palantir.common.base.Throwables; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.UnsafeArg; @@ -92,13 +94,15 @@ public class CassandraKeyValueServiceIntegrationTest extends AbstractKeyValueSer private static final int ONE_HOUR_IN_SECONDS = 60 * 60; private static final TableReference NEVER_SEEN = TableReference.createFromFullyQualifiedName("ns.never_seen"); private static final Cell CELL = Cell.create(PtBytes.toBytes("row"), PtBytes.toBytes("column")); + private static final String ASYNC = "async"; + private static final String SYNC = "sync"; private final String name; @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {"sync", (Function) SynchronousDelegate::new}, - {"async", (Function) AsyncDelegate::new} + {SYNC, (Function) SynchronousDelegate::new}, + {ASYNC, (Function) AsyncDelegate::new} }; return Arrays.asList(data); } @@ -465,8 +469,7 @@ private static IllegalArgumentException getUnrecognizedKeyValueServiceException( return new IllegalArgumentException("Can't run this cassandra-specific test against a non-cassandra KVS"); } - private static class SynchronousDelegate - implements AutoDelegate_CassandraKeyValueService { + private static class SynchronousDelegate implements AutoDelegate_CassandraKeyValueService { private final CassandraKeyValueServiceImpl delegate; SynchronousDelegate(CassandraKeyValueServiceImpl cassandraKeyValueService) { @@ -495,8 +498,10 @@ public CassandraKeyValueServiceImpl delegate() { public Map get(TableReference tableRef, Map timestampByCell) { try { return delegate.getAsync(tableRef, timestampByCell).get(); - } catch (Exception e) { - throw new RuntimeException(e); + } catch (InterruptedException e) { + throw Throwables.rewrapAndThrowUncheckedException(e); + } catch (ExecutionException e) { + throw Throwables.rewrapAndThrowUncheckedException(e.getCause()); } } } diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java index 4b8c2a557c2..9ae2ddc9246 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java @@ -21,8 +21,8 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.function.Function; import java.util.function.Supplier; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.LongStream; @@ -62,8 +62,8 @@ public class CassandraKeyValueServiceTransactionIntegrationTest extends Abstract @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {SYNC, (Function) GetSynchronousDelegate::new}, - {ASYNC, (Function) GetAsyncDelegate::new} + {SYNC, (UnaryOperator) GetSynchronousDelegate::new}, + {ASYNC, (UnaryOperator) GetAsyncDelegate::new} }; return Arrays.asList(data); } @@ -78,11 +78,11 @@ public static Collection data() { @Rule public final TestRule flakeRetryingRule = new FlakeRetryingRule(); private final String name; - private final Function transactionWrapper; + private final UnaryOperator transactionWrapper; public CassandraKeyValueServiceTransactionIntegrationTest( String name, - Function transactionWrapper) { + UnaryOperator transactionWrapper) { super(CASSANDRA, CASSANDRA); this.name = name; this.transactionWrapper = transactionWrapper; diff --git a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java index 95a46833e9c..2e89882b66a 100644 --- a/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java +++ b/atlasdb-cassandra-integration-tests/src/test/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java @@ -19,7 +19,7 @@ import java.util.Arrays; import java.util.Collection; -import java.util.function.Function; +import java.util.function.UnaryOperator; import org.junit.ClassRule; import org.junit.runner.RunWith; @@ -39,21 +39,23 @@ public class CassandraKvsSerializableTransactionTest extends AbstractSerializableTransactionTest { @ClassRule public static final CassandraResource CASSANDRA = new CassandraResource(); + private static final String SYNC = "sync"; + private static final String ASYNC = "async"; @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {"sync", (Function) GetSynchronousDelegate::new}, - {"async", (Function) GetAsyncDelegate::new} + {SYNC, (UnaryOperator) GetSynchronousDelegate::new}, + {ASYNC, (UnaryOperator) GetAsyncDelegate::new} }; return Arrays.asList(data); } - private final Function transactionWrapper; + private final UnaryOperator transactionWrapper; public CassandraKvsSerializableTransactionTest( String name, - Function transactionWrapper) { + UnaryOperator transactionWrapper) { super(CASSANDRA, CASSANDRA); this.transactionWrapper = transactionWrapper; } diff --git a/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java b/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java index 9a13be480e1..5388a5b0b1c 100644 --- a/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java +++ b/atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java @@ -23,6 +23,8 @@ import java.util.function.Predicate; import java.util.function.Supplier; +import javax.annotation.Nullable; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -157,7 +159,7 @@ public static ListenableFuture maybeLogAsync( slowLogPredicate); Futures.addCallback(future, new FutureCallback() { @Override - public void onSuccess(T result) { + public void onSuccess(@Nullable T result) { monitor.registerResult(result); monitor.log(); } diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java index 69c955d375a..605c23c41a4 100644 --- a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java +++ b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java @@ -42,7 +42,9 @@ public Transaction delegate() { public Map get(TableReference tableRef, Set cells) { try { return super.getAsync(tableRef, cells).get(); - } catch (InterruptedException | ExecutionException e) { + } catch (InterruptedException e) { + throw Throwables.rewrapAndThrowUncheckedException(e); + } catch (ExecutionException e) { throw Throwables.rewrapAndThrowUncheckedException(e.getCause()); } } diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManager.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManager.java index 7052171b521..e3ba7a4dbeb 100644 --- a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManager.java +++ b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManager.java @@ -24,4 +24,5 @@ public interface TestTransactionManager extends TransactionManager { Transaction commitAndStartNewTransaction(Transaction txn); Transaction createNewTransaction(); void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler); + void setUnreadableTimestamp(long timestamp); } diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManagerImpl.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManagerImpl.java index 604295f5c78..f48146579f5 100644 --- a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManagerImpl.java +++ b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TestTransactionManagerImpl.java @@ -166,6 +166,7 @@ public long getUnreadableTimestamp() { return unreadableTs.orElseGet(super::getUnreadableTimestamp); } + @Override public void setUnreadableTimestamp(long timestamp) { unreadableTs = Optional.of(timestamp); } diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/WrappingTestTransactionManager.java b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/WrappingTestTransactionManager.java index df20597d29f..25d37217a66 100644 --- a/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/WrappingTestTransactionManager.java +++ b/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/WrappingTestTransactionManager.java @@ -42,4 +42,9 @@ public Transaction createNewTransaction() { public void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler) { delegate.overrideConflictHandlerForTable(table, conflictHandler); } + + @Override + public void setUnreadableTimestamp(long timestamp) { + delegate.setUnreadableTimestamp(timestamp); + } } 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 index ac59acadf02..d09c2867147 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/AtlasDbTestCase.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/AtlasDbTestCase.java @@ -64,7 +64,7 @@ public class AtlasDbTestCase { protected InMemoryTimestampService timestampService; protected ConflictDetectionManager conflictDetectionManager; protected SweepStrategyManager sweepStrategyManager; - protected TestTransactionManagerImpl serializableTxManager; + protected TestTransactionManager serializableTxManager; protected TestTransactionManager txManager; protected TransactionService transactionService; protected TargetedSweeper sweepQueue; @@ -85,7 +85,13 @@ public void setUp() throws Exception { sweepStrategyManager = SweepStrategyManagers.createDefault(keyValueService); sweepQueue = spy(TargetedSweeper.createUninitializedForTest(() -> sweepQueueShards)); + setUpTransactionManagers(); + sweepQueue.initialize(serializableTxManager); + sweepTimestampSupplier = new SpecialTimestampsSupplier( + () -> txManager.getUnreadableTimestamp(), () -> txManager.getImmutableTimestamp()); + } + protected void setUpTransactionManagers() { serializableTxManager = new TestTransactionManagerImpl( metricsManager, keyValueService, @@ -99,10 +105,7 @@ public void setUp() throws Exception { sweepQueue, MoreExecutors.newDirectExecutorService()); - sweepQueue.initialize(serializableTxManager); txManager = new CachingTestTransactionManager(serializableTxManager); - sweepTimestampSupplier = new SpecialTimestampsSupplier( - () -> txManager.getUnreadableTimestamp(), () -> txManager.getImmutableTimestamp()); } protected KeyValueService getBaseKeyValueService() { diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java index 31cee914369..9c65b245751 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java @@ -15,6 +15,9 @@ */ package com.palantir.atlasdb.transaction.impl; + +import static org.junit.Assert.assertEquals; + import java.util.Arrays; import java.util.Collection; import java.util.Map; @@ -22,10 +25,10 @@ import java.util.SortedMap; import java.util.function.BiFunction; import java.util.function.Function; +import java.util.function.UnaryOperator; import org.jmock.Expectations; import org.jmock.Mockery; -import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -54,8 +57,8 @@ public class CachingTransactionTest { @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { - {SYNC, (Function) GetSynchronousDelegate::new}, - {ASYNC, (Function) GetAsyncDelegate::new} + {SYNC, (UnaryOperator) GetSynchronousDelegate::new}, + {ASYNC, (UnaryOperator) GetAsyncDelegate::new} }; return Arrays.asList(data); } @@ -95,8 +98,8 @@ public void testCacheEmptyGets() { } }); - Assert.assertEquals(emptyResults, cachingTransaction.getRows(table, oneRow, oneColumn)); - Assert.assertEquals(emptyResults, cachingTransaction.getRows(table, oneRow, oneColumn)); + assertEquals(emptyResults, cachingTransaction.getRows(table, oneRow, oneColumn)); + assertEquals(emptyResults, cachingTransaction.getRows(table, oneRow, oneColumn)); mockery.assertIsSatisfied(); } @@ -127,8 +130,8 @@ public void testGetRows() { } }); - Assert.assertEquals(oneResult, cachingTransaction.getRows(table, oneRow, oneColumn)); - Assert.assertEquals(oneResult, cachingTransaction.getRows(table, oneRow, oneColumn)); + assertEquals(oneResult, cachingTransaction.getRows(table, oneRow, oneColumn)); + assertEquals(oneResult, cachingTransaction.getRows(table, oneRow, oneColumn)); mockery.assertIsSatisfied(); } @@ -157,8 +160,8 @@ private void testGetCellResults(Cell cell, Map cellValueMap) { final Set cellSet = ImmutableSet.of(cell); mockery.checking(expectationsMapping.get(name).apply(cellSet, cellValueMap)); - Assert.assertEquals(cellValueMap, cachingTransaction.get(table, cellSet)); - Assert.assertEquals(cellValueMap, cachingTransaction.get(table, cellSet)); + assertEquals(cellValueMap, cachingTransaction.get(table, cellSet)); + assertEquals(cellValueMap, cachingTransaction.get(table, cellSet)); mockery.assertIsSatisfied(); } diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index adf658b18d4..bf941fcf86f 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -57,6 +57,7 @@ import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.function.Supplier; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import org.apache.commons.lang3.mutable.MutableInt; @@ -110,7 +111,6 @@ import com.palantir.atlasdb.transaction.ImmutableTransactionConfig; import com.palantir.atlasdb.transaction.TransactionConfig; import com.palantir.atlasdb.transaction.api.AtlasDbConstraintCheckingMode; -import com.palantir.atlasdb.transaction.api.AutoDelegate_TransactionManager; import com.palantir.atlasdb.transaction.api.ConflictHandler; import com.palantir.atlasdb.transaction.api.LockAwareTransactionTask; import com.palantir.atlasdb.transaction.api.PreCommitCondition; @@ -121,7 +121,6 @@ import com.palantir.atlasdb.transaction.api.TransactionFailedRetriableException; import com.palantir.atlasdb.transaction.api.TransactionLockTimeoutException; import com.palantir.atlasdb.transaction.api.TransactionLockTimeoutNonRetriableException; -import com.palantir.atlasdb.transaction.api.TransactionManager; import com.palantir.atlasdb.transaction.api.TransactionReadSentinelBehavior; import com.palantir.atlasdb.transaction.impl.metrics.TransactionOutcomeMetrics; import com.palantir.atlasdb.transaction.impl.metrics.TransactionOutcomeMetricsAssert; @@ -151,18 +150,37 @@ public class SnapshotTransactionTest extends AtlasDbTestCase { private static final String SYNC = "sync"; private static final String ASYNC = "async"; + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Object[][] data = new Object[][] { + {SYNC, (UnaryOperator) GetSynchronousDelegate::new}, + {ASYNC, (UnaryOperator) GetAsyncDelegate::new} + }; + return Arrays.asList(data); + } private final String name; - private final Function transactionWrapper; - private final Map> + private final UnaryOperator transactionWrapper; + private final Map expectationsMapping = - ImmutableMap.>builder() + ImmutableMap.builder() .put(SYNC, SnapshotTransactionTest.this::syncGetExpectation) .put(ASYNC, SnapshotTransactionTest.this::asyncGetExpectation) .build(); + private TransactionConfig transactionConfig; + private final TimestampCache timestampCache = new DefaultTimestampCache( + metricsManager.getRegistry(), () -> AtlasDbConstants.DEFAULT_TIMESTAMP_CACHE_SIZE); + private final ExecutorService getRangesExecutor = Executors.newFixedThreadPool(8); + private final int defaultGetRangesConcurrency = 2; + private final TransactionOutcomeMetrics transactionOutcomeMetrics + = TransactionOutcomeMetrics.create(metricsManager); @FunctionalInterface - interface ExpectationFactory { - Five apply(One one, Two two, Three three, Four four) throws Exception; + interface ExpectationFactory { + Expectations apply( + KeyValueService keyValueService, + Cell cell, + long transactionTs, + LockService lockService) throws InterruptedException; } private Expectations syncGetExpectation( @@ -170,49 +188,24 @@ private Expectations syncGetExpectation( Cell cell, long transactionTs, LockService lockMock) { - try { - return new Expectations() {{ - oneOf(kvMock).get(TABLE, ImmutableMap.of(cell, transactionTs)); - will(throwException(new RuntimeException())); - never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); - }}; - } catch (Exception e) { - throw new RuntimeException(e.getCause()); - } + return new Expectations() {{ + oneOf(kvMock).get(TABLE, ImmutableMap.of(cell, transactionTs)); + will(throwException(new RuntimeException())); + }}; } private Expectations asyncGetExpectation( KeyValueService kvMock, Cell cell, long transactionTs, - LockService lockMock) throws Exception { + LockService lockMock) { return new Expectations() {{ oneOf(kvMock).getAsync(TABLE, ImmutableMap.of(cell, transactionTs)); will(returnValue(Futures.immediateFailedFuture(new RuntimeException()))); - never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); }}; } - private TransactionConfig transactionConfig; - - private final TimestampCache timestampCache = new DefaultTimestampCache( - metricsManager.getRegistry(), () -> AtlasDbConstants.DEFAULT_TIMESTAMP_CACHE_SIZE); - private final ExecutorService getRangesExecutor = Executors.newFixedThreadPool(8); - private final int defaultGetRangesConcurrency = 2; - private final TransactionOutcomeMetrics transactionOutcomeMetrics - = TransactionOutcomeMetrics.create(metricsManager); - private WrappingTestTransactionManager wrappedSerializableTxManager; - - @Parameterized.Parameters(name = "{0}") - public static Collection data() { - Object[][] data = new Object[][] { - {SYNC, (Function) GetSynchronousDelegate::new}, - {ASYNC, (Function) GetAsyncDelegate::new} - }; - return Arrays.asList(data); - } - - public SnapshotTransactionTest(String name, Function transactionWrapper) { + public SnapshotTransactionTest(String name, UnaryOperator transactionWrapper) { this.name = name; this.transactionWrapper = transactionWrapper; } @@ -284,8 +277,6 @@ public void cleanup() {} public void setUp() throws Exception { super.setUp(); - txManager = new WrappingTestTransactionManager(txManager, transactionWrapper); - wrappedSerializableTxManager = new WrappingTestTransactionManager(serializableTxManager, transactionWrapper); transactionConfig = ImmutableTransactionConfig.builder().build(); keyValueService.createTable(TABLE, AtlasDbConstants.GENERIC_TABLE_METADATA); @@ -299,6 +290,13 @@ public void setUp() throws Exception { getTableMetadataForSweepStrategy(SweepStrategy.CONSERVATIVE).persistToBytes()); } + @Override + protected void setUpTransactionManagers() { + super.setUpTransactionManagers(); + txManager = new WrappingTestTransactionManagerImpl(txManager, transactionWrapper); + serializableTxManager = new WrappingTestTransactionManagerImpl(serializableTxManager, transactionWrapper); + } + @Test public void testConcurrentWriteChangedConflicts() throws InterruptedException, ExecutionException { overrideConflictHandlerForTable(TABLE, ConflictHandler.RETRY_ON_VALUE_CHANGED); @@ -339,10 +337,10 @@ public void testImmutableTs() throws Exception { @Test public void testLockAfterGet() throws Exception { byte[] rowName = PtBytes.toBytes("1"); - Mockery m = new Mockery(); - m.setThreadingPolicy(new Synchroniser()); - final KeyValueService kvMock = m.mock(KeyValueService.class); - final LockService lockMock = m.mock(LockService.class); + Mockery mockery = new Mockery(); + mockery.setThreadingPolicy(new Synchroniser()); + final KeyValueService kvMock = mockery.mock(KeyValueService.class); + final LockService lockMock = mockery.mock(LockService.class); LockService lock = MultiDelegateProxy.newProxyInstance(LockService.class, lockService, lockMock); final Cell cell = Cell.create(rowName, rowName); @@ -351,7 +349,10 @@ public void testLockAfterGet() throws Exception { final long transactionTs = timestampService.getFreshTimestamp(); keyValueService.put(TABLE, ImmutableMap.of(cell, PtBytes.EMPTY_BYTE_ARRAY), startTs); - m.checking(expectationsMapping.get(name).apply(kvMock, cell, transactionTs, lockMock)); + mockery.checking(expectationsMapping.get(name).apply(kvMock, cell, transactionTs, lockMock)); + mockery.checking(new Expectations() {{ + never(lockMock).lockWithFullLockResponse(with(LockClient.ANONYMOUS), with(any(LockRequest.class))); + }}); Transaction snapshot = transactionWrapper.apply(new SnapshotTransaction(metricsManager, kvMock, @@ -382,7 +383,7 @@ public void testLockAfterGet() throws Exception { //expected } - m.assertIsSatisfied(); + mockery.assertIsSatisfied(); } @Ignore("Was ignored long ago, and now we need to fix the mocking logic.") @@ -457,7 +458,7 @@ public void testTransactionAtomicity() throws Exception { Random random = new Random(1); final UnstableKeyValueService unstableKvs = new UnstableKeyValueService(keyValueService, random); - final TestTransactionManager unstableTransactionManager = new WrappingTestTransactionManager( + final TestTransactionManager unstableTransactionManager = new WrappingTestTransactionManagerImpl( new TestTransactionManagerImpl( metricsManager, unstableKvs, @@ -600,7 +601,7 @@ public void testGetRowsLocalWritesWithColumnSelection() { byte[] row1Column1Value = BigInteger.valueOf(1).toByteArray(); byte[] row1Column2Value = BigInteger.valueOf(2).toByteArray(); - Transaction snapshotTx = wrappedSerializableTxManager.createNewTransaction(); + Transaction snapshotTx = serializableTxManager.createNewTransaction(); snapshotTx.put(TABLE, ImmutableMap.of( row1Column1, row1Column1Value, row1Column2, row1Column2Value)); @@ -900,7 +901,7 @@ public void noRetryOnExpiredLockTokens() throws InterruptedException { @Test public void commitIfPreCommitConditionSucceeds() { - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict( + serializableTxManager.runTaskWithConditionThrowOnConflict( PreCommitConditions.NO_OP, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); @@ -911,7 +912,7 @@ public void commitIfPreCommitConditionSucceeds() { @Test public void failToCommitIfPreCommitConditionFails() { try { - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict( + serializableTxManager.runTaskWithConditionThrowOnConflict( ALWAYS_FAILS_CONDITION, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); @@ -929,7 +930,7 @@ public void commitWithPreCommitConditionOnRetry() { when(conditionSupplier.get()).thenReturn(ALWAYS_FAILS_CONDITION) .thenReturn(PreCommitConditions.NO_OP); - wrappedSerializableTxManager.runTaskWithConditionWithRetry( + serializableTxManager.runTaskWithConditionWithRetry( conditionSupplier, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); @@ -985,7 +986,7 @@ public void cleanup() {} }; Supplier conditionSupplier = Suppliers.ofInstance(nonRetriableFailure); try { - wrappedSerializableTxManager.runTaskWithConditionWithRetry( + serializableTxManager.runTaskWithConditionWithRetry( conditionSupplier, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); @@ -999,7 +1000,7 @@ public void cleanup() {} @Test public void readTransactionSucceedsIfConditionSucceeds() { - wrappedSerializableTxManager.runTaskWithConditionReadOnly( + serializableTxManager.runTaskWithConditionReadOnly( PreCommitConditions.NO_OP, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); TransactionOutcomeMetricsAssert.assertThat(transactionOutcomeMetrics) @@ -1009,7 +1010,7 @@ public void readTransactionSucceedsIfConditionSucceeds() { @Test public void readTransactionFailsIfConditionFails() { try { - wrappedSerializableTxManager.runTaskWithConditionReadOnly( + serializableTxManager.runTaskWithConditionReadOnly( ALWAYS_FAILS_CONDITION, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); fail(); @@ -1033,7 +1034,7 @@ public void cleanup() { } }; - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict( + serializableTxManager.runTaskWithConditionThrowOnConflict( succeedsCondition, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); @@ -1041,7 +1042,7 @@ public void cleanup() { }); assertThat(counter.intValue(), is(1)); - wrappedSerializableTxManager.runTaskWithConditionReadOnly(succeedsCondition, + serializableTxManager.runTaskWithConditionReadOnly(succeedsCondition, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); assertThat(counter.intValue(), is(2)); } @@ -1062,7 +1063,7 @@ public void cleanup() { }; try { - wrappedSerializableTxManager.runTaskWithConditionThrowOnConflict(failsCondition, (tx, condition) -> { + serializableTxManager.runTaskWithConditionThrowOnConflict(failsCondition, (tx, condition) -> { tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); return null; }); @@ -1073,7 +1074,7 @@ public void cleanup() { assertThat(counter.intValue(), is(1)); try { - wrappedSerializableTxManager.runTaskWithConditionReadOnly(failsCondition, + serializableTxManager.runTaskWithConditionReadOnly(failsCondition, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); fail(); } catch (TransactionFailedRetriableException e) { @@ -1089,14 +1090,14 @@ public void getRowsColumnRangesReturnsInOrderInCaseOfAbortedTxns() { Cell secondCell = Cell.create(row, "b".getBytes()); byte[] value = new byte[1]; - wrappedSerializableTxManager.runTaskWithRetry(tx -> { + serializableTxManager.runTaskWithRetry(tx -> { tx.put(TABLE, ImmutableMap.of(firstCell, value, secondCell, value)); return null; }); // this will write into the DB, because the protocol demands we write before we get a commit timestamp RuntimeException conditionFailure = new RuntimeException(); - assertThatThrownBy(() -> wrappedSerializableTxManager.runTaskWithConditionWithRetry(() -> + assertThatThrownBy(() -> serializableTxManager.runTaskWithConditionWithRetry(() -> new PreCommitCondition() { @Override public void throwIfConditionInvalid(long timestamp) { @@ -1110,7 +1111,7 @@ public void cleanup() {} return null; })).isSameAs(conditionFailure); - List cells = wrappedSerializableTxManager.runTaskReadOnly(tx -> + List cells = serializableTxManager.runTaskReadOnly(tx -> BatchingVisitableView.of(tx.getRowsColumnRange( TABLE, ImmutableList.of(row), @@ -1460,42 +1461,25 @@ private long concurrentlyIncrementValueThousandTimesAndGet() throws InterruptedE * Hack to get reference to underlying {@link SnapshotTransaction}. See how transaction managers are composed at * {@link AtlasDbTestCase#setUp()}. */ - private static SnapshotTransaction unwrapSnapshotTransaction(Transaction cachingTransaction) { - Transaction unwrapped = ((CachingTransaction) cachingTransaction).delegate(); - return (SnapshotTransaction) unwrapped; + private static SnapshotTransaction unwrapSnapshotTransaction(Transaction transaction) { + if (transaction instanceof ForwardingTransaction) + return unwrapSnapshotTransaction(((ForwardingTransaction) transaction).delegate()); + return (SnapshotTransaction) transaction; } - private static class WrappingTestTransactionManager - implements TestTransactionManager, AutoDelegate_TransactionManager { - - private final TestTransactionManager testTransactionManager; + private static class WrappingTestTransactionManagerImpl extends WrappingTestTransactionManager { private final Function transactionWrapper; - WrappingTestTransactionManager( + WrappingTestTransactionManagerImpl( TestTransactionManager testTransactionManager, Function transactionWrapper) { - this.testTransactionManager = testTransactionManager; + super(testTransactionManager); this.transactionWrapper = transactionWrapper; } @Override - public TransactionManager delegate() { - return testTransactionManager; - } - - @Override - public Transaction commitAndStartNewTransaction(Transaction txn) { - return testTransactionManager.commitAndStartNewTransaction(txn); - } - - @Override - public Transaction createNewTransaction() { - return transactionWrapper.apply(testTransactionManager.createNewTransaction()); - } - - @Override - public void overrideConflictHandlerForTable(TableReference table, ConflictHandler conflictHandler) { - testTransactionManager.overrideConflictHandlerForTable(table, conflictHandler); + protected Transaction wrap(Transaction transaction) { + return transactionWrapper.apply(transaction); } } } diff --git a/changelog/@unreleased/pr-4301.v2.yml b/changelog/@unreleased/pr-4301.v2.yml index 948664eb2d1..796b1467108 100644 --- a/changelog/@unreleased/pr-4301.v2.yml +++ b/changelog/@unreleased/pr-4301.v2.yml @@ -1,6 +1,6 @@ type: feature feature: description: | - Transactions can now support asynchronous get operations. This has been implemented for all transaction types. Feature is designed for direct uses of transactions. + `Transaction` can now support asynchronous get operations. Feature is designed for direct users of the `Transaction` api. links: - https://github.com/palantir/atlasdb/pull/4301 From b6c67a0e9ecf4713b9965d9bf86c220e698a3211 Mon Sep 17 00:00:00 2001 From: Stevan Ognjanovic Date: Wed, 16 Oct 2019 13:11:19 +0100 Subject: [PATCH 16/16] Final fixes. --- .../com/palantir/atlasdb/AtlasDbTestCase.java | 10 ++- .../impl/SnapshotTransactionTest.java | 82 ++++++++----------- changelog/@unreleased/pr-4301.v2.yml | 2 +- 3 files changed, 41 insertions(+), 53 deletions(-) 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 index d09c2867147..b813f01bbaf 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/AtlasDbTestCase.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/AtlasDbTestCase.java @@ -91,8 +91,8 @@ public void setUp() throws Exception { () -> txManager.getUnreadableTimestamp(), () -> txManager.getImmutableTimestamp()); } - protected void setUpTransactionManagers() { - serializableTxManager = new TestTransactionManagerImpl( + private void setUpTransactionManagers() { + serializableTxManager = wrapTestTransactionManager(new TestTransactionManagerImpl( metricsManager, keyValueService, timestampService, @@ -103,11 +103,15 @@ protected void setUpTransactionManagers() { conflictDetectionManager, sweepStrategyManager, sweepQueue, - MoreExecutors.newDirectExecutorService()); + MoreExecutors.newDirectExecutorService())); txManager = new CachingTestTransactionManager(serializableTxManager); } + protected TestTransactionManager wrapTestTransactionManager(TestTransactionManager testTransactionManager) { + return testTransactionManager; + } + protected KeyValueService getBaseKeyValueService() { ExecutorService executor = PTExecutors.newSingleThreadExecutor( PTExecutors.newNamedThreadFactory(true)); diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java index bf941fcf86f..11e82234717 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java @@ -150,6 +150,7 @@ public class SnapshotTransactionTest extends AtlasDbTestCase { private static final String SYNC = "sync"; private static final String ASYNC = "async"; + @Parameterized.Parameters(name = "{0}") public static Collection data() { Object[][] data = new Object[][] { @@ -158,6 +159,7 @@ public static Collection data() { }; return Arrays.asList(data); } + private final String name; private final UnaryOperator transactionWrapper; private final Map @@ -166,7 +168,6 @@ public static Collection data() { .put(SYNC, SnapshotTransactionTest.this::syncGetExpectation) .put(ASYNC, SnapshotTransactionTest.this::asyncGetExpectation) .build(); - private TransactionConfig transactionConfig; private final TimestampCache timestampCache = new DefaultTimestampCache( metricsManager.getRegistry(), () -> AtlasDbConstants.DEFAULT_TIMESTAMP_CACHE_SIZE); private final ExecutorService getRangesExecutor = Executors.newFixedThreadPool(8); @@ -174,8 +175,11 @@ public static Collection data() { private final TransactionOutcomeMetrics transactionOutcomeMetrics = TransactionOutcomeMetrics.create(metricsManager); + private TransactionConfig transactionConfig; + @FunctionalInterface interface ExpectationFactory { + Expectations apply( KeyValueService keyValueService, Cell cell, @@ -209,8 +213,8 @@ public SnapshotTransactionTest(String name, UnaryOperator transacti this.name = name; this.transactionWrapper = transactionWrapper; } - private class UnstableKeyValueService implements AutoDelegate_KeyValueService { + private final KeyValueService delegate; private final Random random; @@ -291,10 +295,8 @@ public void setUp() throws Exception { } @Override - protected void setUpTransactionManagers() { - super.setUpTransactionManagers(); - txManager = new WrappingTestTransactionManagerImpl(txManager, transactionWrapper); - serializableTxManager = new WrappingTestTransactionManagerImpl(serializableTxManager, transactionWrapper); + protected TestTransactionManager wrapTestTransactionManager(TestTransactionManager testTransactionManager) { + return new WrappingTestTransactionManagerImpl(testTransactionManager, transactionWrapper); } @Test @@ -711,8 +713,7 @@ private List>> getThoroug final int batchHint = 1; tasks.add(Pair.of("get", (t, heldLocks) -> { - t.get(TABLE_SWEPT_THOROUGH, - ImmutableSet.of(Cell.create(PtBytes.toBytes("row1"), PtBytes.toBytes("column1")))); + t.get(TABLE_SWEPT_THOROUGH, ImmutableSet.of(Cell.create(PtBytes.toBytes("row1"), PtBytes.toBytes("column1")))); return null; })); @@ -752,9 +753,7 @@ private List>> getThoroug tasks.add(Pair.of("getRowsColumnRangeIterator", (t, heldLocks) -> { Collection>> results = - t.getRowsColumnRangeIterator( - TABLE_SWEPT_THOROUGH, - Collections.singleton(PtBytes.toBytes("row1")), + t.getRowsColumnRangeIterator(TABLE_SWEPT_THOROUGH, Collections.singleton(PtBytes.toBytes("row1")), BatchColumnRangeSelection.create(new ColumnRangeSelection(null, null), batchHint)) .values(); results.forEach(Iterators::getLast); @@ -901,23 +900,19 @@ public void noRetryOnExpiredLockTokens() throws InterruptedException { @Test public void commitIfPreCommitConditionSucceeds() { - serializableTxManager.runTaskWithConditionThrowOnConflict( - PreCommitConditions.NO_OP, - (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + serializableTxManager.runTaskWithConditionThrowOnConflict(PreCommitConditions.NO_OP, (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); } @Test public void failToCommitIfPreCommitConditionFails() { try { - serializableTxManager.runTaskWithConditionThrowOnConflict( - ALWAYS_FAILS_CONDITION, - (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + serializableTxManager.runTaskWithConditionThrowOnConflict(ALWAYS_FAILS_CONDITION, (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); fail(); } catch (TransactionFailedRetriableException e) { assertThat(e.getMessage(), containsString("Condition failed")); @@ -930,12 +925,10 @@ public void commitWithPreCommitConditionOnRetry() { when(conditionSupplier.get()).thenReturn(ALWAYS_FAILS_CONDITION) .thenReturn(PreCommitConditions.NO_OP); - serializableTxManager.runTaskWithConditionWithRetry( - conditionSupplier, - (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + serializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); } @Test @@ -986,12 +979,10 @@ public void cleanup() {} }; Supplier conditionSupplier = Suppliers.ofInstance(nonRetriableFailure); try { - serializableTxManager.runTaskWithConditionWithRetry( - conditionSupplier, - (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + serializableTxManager.runTaskWithConditionWithRetry(conditionSupplier, (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); fail(); } catch (TransactionFailedNonRetriableException e) { assertThat(e.getMessage(), containsString("Condition failed")); @@ -1000,8 +991,7 @@ public void cleanup() {} @Test public void readTransactionSucceedsIfConditionSucceeds() { - serializableTxManager.runTaskWithConditionReadOnly( - PreCommitConditions.NO_OP, + serializableTxManager.runTaskWithConditionReadOnly(PreCommitConditions.NO_OP, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); TransactionOutcomeMetricsAssert.assertThat(transactionOutcomeMetrics) .hasSuccessfulCommits(1); @@ -1010,8 +1000,7 @@ public void readTransactionSucceedsIfConditionSucceeds() { @Test public void readTransactionFailsIfConditionFails() { try { - serializableTxManager.runTaskWithConditionReadOnly( - ALWAYS_FAILS_CONDITION, + serializableTxManager.runTaskWithConditionReadOnly(ALWAYS_FAILS_CONDITION, (tx, condition) -> tx.get(TABLE, ImmutableSet.of(TEST_CELL))); fail(); } catch (TransactionFailedRetriableException e) { @@ -1034,12 +1023,10 @@ public void cleanup() { } }; - serializableTxManager.runTaskWithConditionThrowOnConflict( - succeedsCondition, - (tx, condition) -> { - tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); - return null; - }); + serializableTxManager.runTaskWithConditionThrowOnConflict(succeedsCondition, (tx, condition) -> { + tx.put(TABLE, ImmutableMap.of(TEST_CELL, PtBytes.toBytes("value"))); + return null; + }); assertThat(counter.intValue(), is(1)); serializableTxManager.runTaskWithConditionReadOnly(succeedsCondition, @@ -1398,10 +1385,7 @@ private void writeCells(TableReference table, ImmutableMap cellsTo private RowResult readRow(byte[] defaultRow) { Transaction readTransaction = txManager.createNewTransaction(); - SortedMap> allRows = readTransaction.getRows( - TABLE, - ImmutableSet.of(defaultRow), - ColumnSelection.all()); + SortedMap> allRows = readTransaction.getRows(TABLE, ImmutableSet.of(defaultRow), ColumnSelection.all()); return allRows.get(defaultRow); } diff --git a/changelog/@unreleased/pr-4301.v2.yml b/changelog/@unreleased/pr-4301.v2.yml index 796b1467108..c851a9809d5 100644 --- a/changelog/@unreleased/pr-4301.v2.yml +++ b/changelog/@unreleased/pr-4301.v2.yml @@ -1,6 +1,6 @@ type: feature feature: description: | - `Transaction` can now support asynchronous get operations. Feature is designed for direct users of the `Transaction` api. + Transactions can now support asynchronous get operations. This feature is designed for direct users of the `Transaction` API. links: - https://github.com/palantir/atlasdb/pull/4301