rpuch commented on code in PR #1992:
URL: https://github.com/apache/ignite-3/pull/1992#discussion_r1180319280
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/SortedIndexLocker.java:
##########
@@ -155,15 +154,15 @@ public CompletableFuture<Lock> locksForInsert(UUID txId,
BinaryRow tableRow, Row
BinaryTuplePrefix prefix = BinaryTuplePrefix.fromBinaryTuple(key);
- // find next key
- Cursor<IndexRow> cursor = storage.scan(prefix, null,
SortedIndexStorage.GREATER);
+ // Find next key.
+ PeekCursor<IndexRow> cursor = storage.scan(prefix, null,
SortedIndexStorage.GREATER);
- BinaryTuple nextKey;
- if (cursor.hasNext()) {
- nextKey = cursor.next().indexColumns();
- } else { // otherwise INF
- nextKey = POSITIVE_INF;
- }
+ //noinspection ResultOfMethodCallIgnored
+ cursor.hasNext();
+
+ // "peek" works the same as "next" when "hasNext" is called. The only
difference is that it returns null instead of throwing
+ // an exception, if nothing is found.
+ BinaryTuple nextKey = indexKey(cursor.peek());
Review Comment:
The code 'after' seems to be equivalent to the code 'before', but the code
'before' was arguably more straightforward. Are you sure this actually improves
readability?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/table/ItTableScanTest.java:
##########
@@ -178,9 +181,8 @@ public void testInsertDuringScan() throws Exception {
subscription.request(1);
- waitForCondition(() -> !scannedRows.isEmpty(), 10_000);
+ assertTrue(waitForCondition(() -> !scannedRows.isEmpty(), 10_000));
- assertEquals(1, scannedRows.size());
Review Comment:
The change is not equivalent: earlier it checked that there is exactly 1
element, now it will allow any non-zero number of elements
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/table/ItTableScanTest.java:
##########
@@ -590,14 +513,71 @@ public void testScanWithUpperBound() throws Exception {
null
);
- ArrayList<BinaryRow> scannedRows2 = scanAllRows(publisher2);
+ List<BinaryRow> scannedRows2 = scanAllRows(publisher2);
assertEquals(5, scannedRows2.size());
log.info("Result of scanning after insert rows: " +
scannedRows2.stream().map(binaryRow -> rowToString(binaryRow))
.collect(Collectors.joining(", ")));
}
+ @Test
+ public void testPhantomReads() throws Exception {
+ int iterations = 10;
+
+ // "for" is better at detecting data races than RepeatedTest.
+ for (int i = 0; i < iterations; i++) {
+ KeyValueView<Tuple, Tuple> kvView = table.keyValueView();
+
+ UUID sortedIndexId = getSortedIndexId();
+
+ int partId = 0;
+
+ InternalTransaction tx = startTxWithEnlistedPartition(partId);
+
+ try {
+ IgniteBiTuple<ClusterNode, Long> leaderWithTerm =
tx.enlistedNodeAndTerm(new TablePartitionId(table.tableId(), partId));
+
+ PrimaryReplica recipient = new
PrimaryReplica(leaderWithTerm.get1(), leaderWithTerm.get2());
+
+ Publisher<BinaryRow> publisher = internalTable.scan(partId,
tx.id(), recipient, sortedIndexId, null, null, 0, null);
+
+ // Non-thread-safe collection is fine, HB is guaranteed by
"Thread#join" inside of "runRace".
Review Comment:
How about just replacing the collection with a thread-safe one, so that the
thread-safety question do not even arise? It's a test, performance does not
seem to be critical here. Then the comment is not needed at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
