You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ibessonov (via GitHub)" <gi...@apache.org> on 2023/04/27 11:23:07 UTC
[GitHub] [ignite-3] ibessonov opened a new pull request, #1992: IGNITE-19302 Phantom reads fixed
ibessonov opened a new pull request, #1992:
URL: https://github.com/apache/ignite-3/pull/1992
https://issues.apache.org/jira/browse/IGNITE-19302
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1992: IGNITE-19302 Phantom reads fixed
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #1992:
URL: https://github.com/apache/ignite-3/pull/1992#discussion_r1180344448
##########
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:
Old code was a scenario copy-pasted twice, I removed the copy-paste.
What else is equivalent? Replacement of "hasNext()" with "peek() != null" is not an equivalent change, it's a fix
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1992: IGNITE-19302 Phantom reads fixed
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #1992:
URL: https://github.com/apache/ignite-3/pull/1992#discussion_r1180342821
##########
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:
I don't like using thread-safe collections when no one needs them, I consider this a bad practice.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov merged pull request #1992: IGNITE-19302 Phantom reads fixed
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov merged PR #1992:
URL: https://github.com/apache/ignite-3/pull/1992
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1992: IGNITE-19302 Phantom reads fixed
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #1992:
URL: https://github.com/apache/ignite-3/pull/1992#discussion_r1180485815
##########
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:
> "peek" works the same as "next" when "hasNext" is called
So I concluded that the code after the change works in the same way as the code before the change. If this is not the case, could you please describe a scenario that would demonstrate a difference?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1992: IGNITE-19302 Phantom reads fixed
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1992: IGNITE-19302 Phantom reads fixed
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #1992:
URL: https://github.com/apache/ignite-3/pull/1992#discussion_r1182260512
##########
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:
My comment is not relevant anymore, please ignore it
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org