[GitHub] [ignite-3] ibessonov commented on a diff in pull request #886: IGNITE-17008 Add support for NextLink field in VersionChain

[GitBox <gi...@apache.org>]

ibessonov commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r911728747


##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
             throw new NoUncommittedVersionException();
         }
 
-        RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
-        assert currentVersion.isUncommitted();
+        RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);

Review Comment:
   I believe that this logic is already changed in master, can you check? Aborting a row that had no write intent is not an exceptional situation anymore



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/VersionChainDataIo.java:
##########
@@ -68,14 +68,16 @@ protected void writeRowData(long pageAddr, int dataOff, int payloadSize, Version
         addr += TransactionIds.writeTransactionId(addr, 0, row.transactionId());
 
         addr += PartitionlessLinks.writeToMemory(addr, row.headLink());
+        addr += PartitionlessLinks.writeToMemory(addr, row.nextLink());
     }
 
     /** {@inheritDoc} */
     @Override
     protected void writeFragmentData(VersionChain row, ByteBuffer buf, int rowOff, int payloadSize) {
         assertPageType(buf);
 
-        throw new UnsupportedOperationException("Splitting version chain rows to fragments is ridiculous!");
+        throw new IllegalStateException("Version chains must never be split to fragments, this should be guaranteed "

Review Comment:
   I'm confident that "IllegalStateException" is wildly misused in Ignite. UnsupportedOperationException looked more appropriate



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
             throw new NoUncommittedVersionException();
         }
 
-        RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
-        assert currentVersion.isUncommitted();
+        RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
+        assert latestVersion.isUncommitted();
+
+        removeRowVersion(latestVersion);
 
-        removeRowVersion(currentVersion);
+        if (latestVersion.hasNextLink()) {
+            // This load can be avoided, see the comment below.
+            RowVersion preLatestVersion = readPredecessor(latestVersion);

Review Comment:
   Using a word "predecessor" in conjuntion with "next" later in the code is confusing. Can we come up with something better?



-- 
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