sashapolo commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945546801
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -27,7 +27,14 @@
import org.jetbrains.annotations.Nullable;
/**
- * Multi-versioned partition storage.
+ * Multi-versioned partition storage. Maps RowId to a structures called
"Version Chains". Each version chain is logically a stack of
+ * elements with current structure:
Review Comment:
```suggestion
* elements with the following structure:
```
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java:
##########
@@ -119,45 +119,48 @@ protected BinaryRow abortWrite(RowId rowId) {
return storage.runConsistently(() -> storage.abortWrite(rowId));
}
+ /**
+ * Returns a partition id that should be used to create a partition
instance.
+ */
+ protected int partitionId() {
Review Comment:
These methods are never overridden. What's the point of having them?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChain.java:
##########
@@ -94,36 +73,16 @@ public long nextLink() {
return nextLink;
}
- public long newestCommittedPartitionlessLink() {
+ public long newestCommittedLink() {
Review Comment:
This and many other methods don't have any javadocs
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/VersionChainIo.java:
##########
@@ -17,15 +17,142 @@
package org.apache.ignite.internal.storage.pagememory.mv.io;
+import static org.apache.ignite.internal.pagememory.util.PageUtils.getLong;
+import static org.apache.ignite.internal.pagememory.util.PageUtils.putLong;
+import static
org.apache.ignite.internal.storage.pagememory.mv.PartitionlessLinks.PARTITIONLESS_LINK_SIZE_BYTES;
+import static
org.apache.ignite.internal.storage.pagememory.mv.PartitionlessLinks.readPartitionlessLink;
+import static
org.apache.ignite.internal.storage.pagememory.mv.PartitionlessLinks.writePartitionlessLink;
+import static
org.apache.ignite.internal.storage.pagememory.mv.VersionChain.NULL_UUID_COMPONENT;
+
+import java.util.UUID;
+import org.apache.ignite.internal.pagememory.tree.io.BplusIo;
+import org.apache.ignite.internal.pagememory.util.PageUtils;
+import org.apache.ignite.internal.storage.RowId;
+import org.apache.ignite.internal.storage.pagememory.mv.VersionChain;
+import org.apache.ignite.internal.storage.pagememory.mv.VersionChainKey;
+import org.apache.ignite.internal.storage.pagememory.mv.VersionChainTree;
+
/**
- * Interface for VersionChain B+Tree-related IO.
+ * Interface for VersionChain B+Tree-related IO. Defines a following data
loayout:
Review Comment:
```suggestion
* Interface for VersionChain B+Tree-related IO. Defines a following data
layout:
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PartitionlessLinks.java:
##########
@@ -38,105 +42,53 @@ public class PartitionlessLinks {
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Converts a full link to partitionless link by removing the partition ID
from it.
+ * Reads a partitionless link from the memory.
*
- * @param link full link
- * @return partitionless link
- * @see PageIdUtils#link(long, int)
+ * @param pageAddr Page address.
+ * @param offset Data offset.
+ * @return Partitionless link.
*/
- public static long removePartitionIdFromLink(long link) {
- return ((link >> PageIdUtils.PART_ID_SIZE) & 0x0000FFFF00000000L)
- | (link & 0xFFFFFFFFL);
- }
-
- /**
- * Converts a partitionless link to a full link by inserting partition ID
at the corresponding position.
- *
- * @param partitionlessLink link without partition
- * @param partitionId partition ID to insert
- * @return full link
- * @see PageIdUtils#link(long, int)
- */
- public static long addPartitionIdToPartititionlessLink(long
partitionlessLink, int partitionId) {
- return (partitionlessLink << PageIdUtils.PART_ID_SIZE) &
0xFFFF000000000000L
- | ((((long) partitionId) << PageIdUtils.PAGE_IDX_SIZE) &
0xFFFF00000000L)
- | (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Returns high 16 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return high 16 bits
- * @see #assemble(short, int)
- */
- public static short high16Bits(long partitionlessLink) {
- return (short) (partitionlessLink >> Integer.SIZE);
- }
-
- /**
- * Returns low 32 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return low 32 bits
- * @see #assemble(short, int)
- */
- public static int low32Bits(long partitionlessLink) {
- return (int) (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Assembles a partitionless link from high 16 bits and 32 low bits of its
representation.
- *
- * @param high16 high 16 bits
- * @param low32 low 32 bits
- * @return reconstructed partitionless link
- * @see #high16Bits(long)
- * @see #low32Bits(long)
- */
- public static long assemble(short high16, int low32) {
- return ((((long) high16) & 0xFFFF) << 32)
- | (((long) low32) & 0xFFFFFFFFL);
- }
+ public static long readPartitionlessLink(int partitionId, long pageAddr,
int offset) {
+ int tag = 0xFFFF & getShort(pageAddr, offset);
+ int pageIdx = getInt(pageAddr, offset + Short.BYTES);
- static long readFromMemory(long pageAddr, int offset) {
- short nextLinkHigh16 = getShort(pageAddr, offset);
- int nextLinkLow32 = getInt(pageAddr, offset + Short.BYTES);
+ // Links to metapages are impossible. For the sake of simplicity,
NULL_LINK is returned in this case.
+ if (pageIdx == 0) {
+ assert tag == 0 : tag;
- return assemble(nextLinkHigh16, nextLinkLow32);
- }
+ return RowVersion.NULL_LINK;
+ }
- static long readFromBuffer(ByteBuffer buffer) {
- short nextLinkHigh16 = buffer.getShort();
- int nextLinkLow32 = buffer.getInt();
+ long pageId = pageId(partitionId, (byte) tag, pageIdx);
- return assemble(nextLinkHigh16, nextLinkLow32);
+ return link(pageId, tag >>> 8);
}
/**
* Writes a partitionless link to memory: first high 2 bytes, then low 4
bytes.
*
* @param addr address in memory where to start
- * @param partitionlessLink the link to write
+ * @param link the link to write
* @return number of bytes written (equal to {@link
#PARTITIONLESS_LINK_SIZE_BYTES})
*/
- public static long writeToMemory(long addr, long partitionlessLink) {
- putShort(addr, 0, PartitionlessLinks.high16Bits(partitionlessLink));
- addr += Short.BYTES;
- putInt(addr, 0, PartitionlessLinks.low32Bits(partitionlessLink));
+ public static long writePartitionlessLink(long addr, long link) {
+ putShort(addr, 0, (short) tag(link));
+
+ putInt(addr + Short.BYTES, 0, pageIndex(link));
- return PartitionlessLinks.PARTITIONLESS_LINK_SIZE_BYTES;
+ return PARTITIONLESS_LINK_SIZE_BYTES;
Review Comment:
why do you need to return a value here if it's a constant?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/TxIdMismatchException.java:
##########
@@ -17,10 +17,29 @@
package org.apache.ignite.internal.storage;
+import java.util.UUID;
import org.apache.ignite.lang.IgniteException;
/**
* Exception class that describes the situation when two independent
transactions attempt to write values for the same key.
*/
public class TxIdMismatchException extends IgniteException {
+ /** Conflicting transaction id. */
+ private final UUID transactionId;
Review Comment:
I think it makes sense to provide both conflicting IDs
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PartitionlessLinks.java:
##########
@@ -38,105 +42,53 @@ public class PartitionlessLinks {
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Converts a full link to partitionless link by removing the partition ID
from it.
+ * Reads a partitionless link from the memory.
*
- * @param link full link
- * @return partitionless link
- * @see PageIdUtils#link(long, int)
+ * @param pageAddr Page address.
+ * @param offset Data offset.
+ * @return Partitionless link.
*/
- public static long removePartitionIdFromLink(long link) {
- return ((link >> PageIdUtils.PART_ID_SIZE) & 0x0000FFFF00000000L)
- | (link & 0xFFFFFFFFL);
- }
-
- /**
- * Converts a partitionless link to a full link by inserting partition ID
at the corresponding position.
- *
- * @param partitionlessLink link without partition
- * @param partitionId partition ID to insert
- * @return full link
- * @see PageIdUtils#link(long, int)
- */
- public static long addPartitionIdToPartititionlessLink(long
partitionlessLink, int partitionId) {
- return (partitionlessLink << PageIdUtils.PART_ID_SIZE) &
0xFFFF000000000000L
- | ((((long) partitionId) << PageIdUtils.PAGE_IDX_SIZE) &
0xFFFF00000000L)
- | (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Returns high 16 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return high 16 bits
- * @see #assemble(short, int)
- */
- public static short high16Bits(long partitionlessLink) {
- return (short) (partitionlessLink >> Integer.SIZE);
- }
-
- /**
- * Returns low 32 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return low 32 bits
- * @see #assemble(short, int)
- */
- public static int low32Bits(long partitionlessLink) {
- return (int) (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Assembles a partitionless link from high 16 bits and 32 low bits of its
representation.
- *
- * @param high16 high 16 bits
- * @param low32 low 32 bits
- * @return reconstructed partitionless link
- * @see #high16Bits(long)
- * @see #low32Bits(long)
- */
- public static long assemble(short high16, int low32) {
- return ((((long) high16) & 0xFFFF) << 32)
- | (((long) low32) & 0xFFFFFFFFL);
- }
+ public static long readPartitionlessLink(int partitionId, long pageAddr,
int offset) {
+ int tag = 0xFFFF & getShort(pageAddr, offset);
+ int pageIdx = getInt(pageAddr, offset + Short.BYTES);
- static long readFromMemory(long pageAddr, int offset) {
- short nextLinkHigh16 = getShort(pageAddr, offset);
- int nextLinkLow32 = getInt(pageAddr, offset + Short.BYTES);
+ // Links to metapages are impossible. For the sake of simplicity,
NULL_LINK is returned in this case.
+ if (pageIdx == 0) {
+ assert tag == 0 : tag;
- return assemble(nextLinkHigh16, nextLinkLow32);
- }
+ return RowVersion.NULL_LINK;
+ }
- static long readFromBuffer(ByteBuffer buffer) {
- short nextLinkHigh16 = buffer.getShort();
- int nextLinkLow32 = buffer.getInt();
+ long pageId = pageId(partitionId, (byte) tag, pageIdx);
- return assemble(nextLinkHigh16, nextLinkLow32);
+ return link(pageId, tag >>> 8);
}
/**
* Writes a partitionless link to memory: first high 2 bytes, then low 4
bytes.
*
* @param addr address in memory where to start
- * @param partitionlessLink the link to write
+ * @param link the link to write
* @return number of bytes written (equal to {@link
#PARTITIONLESS_LINK_SIZE_BYTES})
*/
- public static long writeToMemory(long addr, long partitionlessLink) {
- putShort(addr, 0, PartitionlessLinks.high16Bits(partitionlessLink));
- addr += Short.BYTES;
- putInt(addr, 0, PartitionlessLinks.low32Bits(partitionlessLink));
+ public static long writePartitionlessLink(long addr, long link) {
+ putShort(addr, 0, (short) tag(link));
Review Comment:
I wonder why does `tag` method return an `int`, if the tag is 2 bytes?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PartitionlessLinks.java:
##########
@@ -38,105 +42,53 @@ public class PartitionlessLinks {
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Converts a full link to partitionless link by removing the partition ID
from it.
+ * Reads a partitionless link from the memory.
*
- * @param link full link
- * @return partitionless link
- * @see PageIdUtils#link(long, int)
+ * @param pageAddr Page address.
+ * @param offset Data offset.
+ * @return Partitionless link.
*/
- public static long removePartitionIdFromLink(long link) {
- return ((link >> PageIdUtils.PART_ID_SIZE) & 0x0000FFFF00000000L)
- | (link & 0xFFFFFFFFL);
- }
-
- /**
- * Converts a partitionless link to a full link by inserting partition ID
at the corresponding position.
- *
- * @param partitionlessLink link without partition
- * @param partitionId partition ID to insert
- * @return full link
- * @see PageIdUtils#link(long, int)
- */
- public static long addPartitionIdToPartititionlessLink(long
partitionlessLink, int partitionId) {
- return (partitionlessLink << PageIdUtils.PART_ID_SIZE) &
0xFFFF000000000000L
- | ((((long) partitionId) << PageIdUtils.PAGE_IDX_SIZE) &
0xFFFF00000000L)
- | (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Returns high 16 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return high 16 bits
- * @see #assemble(short, int)
- */
- public static short high16Bits(long partitionlessLink) {
- return (short) (partitionlessLink >> Integer.SIZE);
- }
-
- /**
- * Returns low 32 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return low 32 bits
- * @see #assemble(short, int)
- */
- public static int low32Bits(long partitionlessLink) {
- return (int) (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Assembles a partitionless link from high 16 bits and 32 low bits of its
representation.
- *
- * @param high16 high 16 bits
- * @param low32 low 32 bits
- * @return reconstructed partitionless link
- * @see #high16Bits(long)
- * @see #low32Bits(long)
- */
- public static long assemble(short high16, int low32) {
- return ((((long) high16) & 0xFFFF) << 32)
- | (((long) low32) & 0xFFFFFFFFL);
- }
+ public static long readPartitionlessLink(int partitionId, long pageAddr,
int offset) {
+ int tag = 0xFFFF & getShort(pageAddr, offset);
+ int pageIdx = getInt(pageAddr, offset + Short.BYTES);
- static long readFromMemory(long pageAddr, int offset) {
- short nextLinkHigh16 = getShort(pageAddr, offset);
- int nextLinkLow32 = getInt(pageAddr, offset + Short.BYTES);
+ // Links to metapages are impossible. For the sake of simplicity,
NULL_LINK is returned in this case.
+ if (pageIdx == 0) {
+ assert tag == 0 : tag;
- return assemble(nextLinkHigh16, nextLinkLow32);
- }
+ return RowVersion.NULL_LINK;
+ }
- static long readFromBuffer(ByteBuffer buffer) {
- short nextLinkHigh16 = buffer.getShort();
- int nextLinkLow32 = buffer.getInt();
+ long pageId = pageId(partitionId, (byte) tag, pageIdx);
Review Comment:
Since you are deconstructing the tag into parts, it would be nice to assign
them to some variables
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/RowId.java:
##########
@@ -17,14 +17,96 @@
package org.apache.ignite.internal.storage;
+import java.util.UUID;
+import org.apache.ignite.internal.tx.Timestamp;
+
/**
- * Interface that represents row id in primary index of the table.
+ * Class that represents row id in primary index of the table. Contains a
timestamp-based UUID and a partition id.
*
* @see MvPartitionStorage
*/
-public interface RowId {
+public final class RowId {
+ /** Partition id. */
+ private final short partitionId;
+
+ /** Unique id. */
+ private final UUID uuid;
+
+ /**
+ * Create a row id with the UUID value based on {@link Timestamp}.
+ *
+ * @param partitionId Partition id.
+ */
+ public RowId(int partitionId) {
+ this(partitionId, Timestamp.nextVersion().toUuid());
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param partitionId Partition id.
+ * @param mostSignificantBits UUID's most significant bits.
+ * @param leastSignificantBits UUID's least significant bits.
+ */
+ public RowId(int partitionId, long mostSignificantBits, long
leastSignificantBits) {
+ this(partitionId, new UUID(mostSignificantBits, leastSignificantBits));
+ }
+
+ private RowId(int partitionId, UUID uuid) {
+ this.partitionId = (short) (partitionId & 0xFFFF);
+ this.uuid = uuid;
+ }
+
/**
* Returns a partition id for current row id.
*/
- int partitionId();
+ public int partitionId() {
+ return partitionId & 0xFFFF;
+ }
+
+ /**
+ * Returns the most significant 64 bits of row id's UUID.
+ */
+ public long mostSignificantBits() {
+ return uuid.getMostSignificantBits();
+ }
+
+ /**
+ * Returns the least significant 64 bits of row id's UUID.
+ */
+ public long leastSignificantBits() {
+ return uuid.getLeastSignificantBits();
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ RowId rowId = (RowId) o;
+
+ if (partitionId != rowId.partitionId) {
+ return false;
+ }
+ return uuid.equals(rowId.uuid);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public int hashCode() {
+ int result = partitionId;
+ result = 31 * result + uuid.hashCode();
+ return result;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public String toString() {
+ return "RowId [partitionId=" + (partitionId & 0xFFFF) + ", uuid=" +
uuid + ']';
Review Comment:
`partitionId & 0xFFFF` can be replaced with a getter
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PartitionlessLinks.java:
##########
@@ -38,105 +42,53 @@ public class PartitionlessLinks {
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Converts a full link to partitionless link by removing the partition ID
from it.
+ * Reads a partitionless link from the memory.
*
- * @param link full link
- * @return partitionless link
- * @see PageIdUtils#link(long, int)
+ * @param pageAddr Page address.
+ * @param offset Data offset.
+ * @return Partitionless link.
*/
- public static long removePartitionIdFromLink(long link) {
- return ((link >> PageIdUtils.PART_ID_SIZE) & 0x0000FFFF00000000L)
- | (link & 0xFFFFFFFFL);
- }
-
- /**
- * Converts a partitionless link to a full link by inserting partition ID
at the corresponding position.
- *
- * @param partitionlessLink link without partition
- * @param partitionId partition ID to insert
- * @return full link
- * @see PageIdUtils#link(long, int)
- */
- public static long addPartitionIdToPartititionlessLink(long
partitionlessLink, int partitionId) {
- return (partitionlessLink << PageIdUtils.PART_ID_SIZE) &
0xFFFF000000000000L
- | ((((long) partitionId) << PageIdUtils.PAGE_IDX_SIZE) &
0xFFFF00000000L)
- | (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Returns high 16 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return high 16 bits
- * @see #assemble(short, int)
- */
- public static short high16Bits(long partitionlessLink) {
- return (short) (partitionlessLink >> Integer.SIZE);
- }
-
- /**
- * Returns low 32 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return low 32 bits
- * @see #assemble(short, int)
- */
- public static int low32Bits(long partitionlessLink) {
- return (int) (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Assembles a partitionless link from high 16 bits and 32 low bits of its
representation.
- *
- * @param high16 high 16 bits
- * @param low32 low 32 bits
- * @return reconstructed partitionless link
- * @see #high16Bits(long)
- * @see #low32Bits(long)
- */
- public static long assemble(short high16, int low32) {
- return ((((long) high16) & 0xFFFF) << 32)
- | (((long) low32) & 0xFFFFFFFFL);
- }
+ public static long readPartitionlessLink(int partitionId, long pageAddr,
int offset) {
+ int tag = 0xFFFF & getShort(pageAddr, offset);
+ int pageIdx = getInt(pageAddr, offset + Short.BYTES);
- static long readFromMemory(long pageAddr, int offset) {
- short nextLinkHigh16 = getShort(pageAddr, offset);
- int nextLinkLow32 = getInt(pageAddr, offset + Short.BYTES);
+ // Links to metapages are impossible. For the sake of simplicity,
NULL_LINK is returned in this case.
+ if (pageIdx == 0) {
+ assert tag == 0 : tag;
- return assemble(nextLinkHigh16, nextLinkLow32);
- }
+ return RowVersion.NULL_LINK;
+ }
- static long readFromBuffer(ByteBuffer buffer) {
- short nextLinkHigh16 = buffer.getShort();
- int nextLinkLow32 = buffer.getInt();
+ long pageId = pageId(partitionId, (byte) tag, pageIdx);
- return assemble(nextLinkHigh16, nextLinkLow32);
+ return link(pageId, tag >>> 8);
}
/**
* Writes a partitionless link to memory: first high 2 bytes, then low 4
bytes.
*
* @param addr address in memory where to start
- * @param partitionlessLink the link to write
+ * @param link the link to write
* @return number of bytes written (equal to {@link
#PARTITIONLESS_LINK_SIZE_BYTES})
*/
- public static long writeToMemory(long addr, long partitionlessLink) {
- putShort(addr, 0, PartitionlessLinks.high16Bits(partitionlessLink));
- addr += Short.BYTES;
- putInt(addr, 0, PartitionlessLinks.low32Bits(partitionlessLink));
+ public static long writePartitionlessLink(long addr, long link) {
+ putShort(addr, 0, (short) tag(link));
+
+ putInt(addr + Short.BYTES, 0, pageIndex(link));
- return PartitionlessLinks.PARTITIONLESS_LINK_SIZE_BYTES;
+ return PARTITIONLESS_LINK_SIZE_BYTES;
}
/**
* Writes a partitionless link to a buffer: first high 2 bytes, then low 4
bytes.
*
- * @param buffer where to write
- * @param partitionlessLink the link to write
+ * @param buffer Where to write.
Review Comment:
```suggestion
* @param buffer Buffer to write into.
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PartitionlessLinks.java:
##########
@@ -38,105 +42,53 @@ public class PartitionlessLinks {
public static final int PARTITIONLESS_LINK_SIZE_BYTES = 6;
/**
- * Converts a full link to partitionless link by removing the partition ID
from it.
+ * Reads a partitionless link from the memory.
*
- * @param link full link
- * @return partitionless link
- * @see PageIdUtils#link(long, int)
+ * @param pageAddr Page address.
+ * @param offset Data offset.
+ * @return Partitionless link.
*/
- public static long removePartitionIdFromLink(long link) {
- return ((link >> PageIdUtils.PART_ID_SIZE) & 0x0000FFFF00000000L)
- | (link & 0xFFFFFFFFL);
- }
-
- /**
- * Converts a partitionless link to a full link by inserting partition ID
at the corresponding position.
- *
- * @param partitionlessLink link without partition
- * @param partitionId partition ID to insert
- * @return full link
- * @see PageIdUtils#link(long, int)
- */
- public static long addPartitionIdToPartititionlessLink(long
partitionlessLink, int partitionId) {
- return (partitionlessLink << PageIdUtils.PART_ID_SIZE) &
0xFFFF000000000000L
- | ((((long) partitionId) << PageIdUtils.PAGE_IDX_SIZE) &
0xFFFF00000000L)
- | (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Returns high 16 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return high 16 bits
- * @see #assemble(short, int)
- */
- public static short high16Bits(long partitionlessLink) {
- return (short) (partitionlessLink >> Integer.SIZE);
- }
-
- /**
- * Returns low 32 bits of the 6 bytes representing a partitionless link.
- *
- * @param partitionlessLink link without partition info
- * @return low 32 bits
- * @see #assemble(short, int)
- */
- public static int low32Bits(long partitionlessLink) {
- return (int) (partitionlessLink & PageIdUtils.PAGE_IDX_MASK);
- }
-
- /**
- * Assembles a partitionless link from high 16 bits and 32 low bits of its
representation.
- *
- * @param high16 high 16 bits
- * @param low32 low 32 bits
- * @return reconstructed partitionless link
- * @see #high16Bits(long)
- * @see #low32Bits(long)
- */
- public static long assemble(short high16, int low32) {
- return ((((long) high16) & 0xFFFF) << 32)
- | (((long) low32) & 0xFFFFFFFFL);
- }
+ public static long readPartitionlessLink(int partitionId, long pageAddr,
int offset) {
+ int tag = 0xFFFF & getShort(pageAddr, offset);
Review Comment:
kind of a nitpick but we usually apply the mask on the right side of the
expression
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChain.java:
##########
@@ -49,33 +39,22 @@ public class VersionChain extends VersionChainLink
implements Storable {
private final long headLink;
Review Comment:
Not related to this PR, but `latest` sounds confusing, I think it would be
better called `most recent`
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChain.java:
##########
@@ -49,33 +39,22 @@ public class VersionChain extends VersionChainLink
implements Storable {
private final long headLink;
/**
- * Link to the pre-latest version ({@link RowVersion#NULL_LINK} if there
is just one version).
+ * Link to the pre-latest version ({@link RowVersion#isNullLink(long)} is
{@code true} if there is just one version).
Review Comment:
`RowVersion#isNullLink` no longer exists
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/ScanVersionChainByTimestamp.java:
##########
@@ -32,6 +33,7 @@
* version it needs, it switches to traversing the slots comprising the
version (because it might be fragmented).
*/
class ScanVersionChainByTimestamp implements PageMemoryTraversal<Timestamp> {
+ private final int partitionId;
Review Comment:
missing linebreak
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChain.java:
##########
@@ -49,33 +39,22 @@ public class VersionChain extends VersionChainLink
implements Storable {
private final long headLink;
/**
- * Link to the pre-latest version ({@link RowVersion#NULL_LINK} if there
is just one version).
+ * Link to the pre-latest version ({@link RowVersion#isNullLink(long)} is
{@code true} if there is just one version).
*/
private final long nextLink;
/**
* Constructs a VersionChain without a transaction ID.
*/
- public static VersionChain withoutTxId(int partitionId, long link, long
headLink, long nextLink) {
- return new VersionChain(partitionId, link, null, headLink, nextLink);
+ public static VersionChain withoutTxId(RowId rowId, long headLink, long
nextLink) {
Review Comment:
what's the point of this method?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChainKey.java:
##########
@@ -18,10 +18,27 @@
package org.apache.ignite.internal.storage.pagememory.mv;
import org.apache.ignite.internal.storage.RowId;
-import org.apache.ignite.lang.IgniteInternalException;
/**
- * Thrown when trying to do a modification at {@link RowId} that has already
become invalid for writes.
+ * Search key for the {@link VersionChainTree}.
*/
-class RowIdIsInvalidForModificationsException extends IgniteInternalException {
+public class VersionChainKey {
+ /** Row id. */
+ protected final RowId rowId;
Review Comment:
this field can be `private` since there's a public getter
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChainTree.java:
##########
@@ -73,30 +70,24 @@ public VersionChainTree(
reuseList
);
- dataPageReader = new VersionChainDataPageReader(pageMem, grpId,
IoStatisticsHolderNoOp.INSTANCE);
-
setIos(VersionChainInnerIo.VERSIONS, VersionChainLeafIo.VERSIONS,
VersionChainMetaIo.VERSIONS);
initTree(initNew);
}
/** {@inheritDoc} */
@Override
- protected int compare(BplusIo<VersionChainLink> io, long pageAddr, int
idx, VersionChainLink row) {
+ protected int compare(BplusIo<VersionChainKey> io, long pageAddr, int idx,
VersionChainKey row) {
VersionChainIo versionChainIo = (VersionChainIo) io;
- long thisLink = versionChainIo.link(pageAddr, idx);
- long thatLink = row.link();
- return Long.compare(thisLink, thatLink);
+ return versionChainIo.compareTo(pageAddr, idx, row);
Review Comment:
I think `compare` would be a better name for this method
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/VersionChainLeafIo.java:
##########
@@ -44,43 +41,25 @@ public class VersionChainLeafIo extends
BplusLeafIo<VersionChainLink> implements
*
* @param ver Page format version.
*/
- protected VersionChainLeafIo(int ver) {
- super(T_VERSION_CHAIN_LEAF_IO, ver, VersionChainLink.SIZE_IN_BYTES);
+ VersionChainLeafIo(int ver) {
Review Comment:
shall it be `private`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/VersionChainInnerIo.java:
##########
@@ -44,43 +41,25 @@ public class VersionChainInnerIo extends
BplusInnerIo<VersionChainLink> implemen
*
* @param ver Page format version.
*/
- protected VersionChainInnerIo(int ver) {
- super(T_VERSION_CHAIN_INNER_IO, ver, true,
VersionChainLink.SIZE_IN_BYTES);
+ VersionChainInnerIo(int ver) {
Review Comment:
Should it be `private` since it's currently a singleton?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -50,51 +49,44 @@ public abstract class AbstractPageMemoryMvPartitionStorage
implements MvPartitio
private static final Predicate<BinaryRow> MATCH_ALL = row -> true;
private static final Predicate<Timestamp> ALWAYS_LOAD_VALUE = timestamp ->
true;
- private static final Predicate<Timestamp> NEVER_LOAD_VALUE = timestamp ->
false;
- private static final Predicate<Timestamp> LOAD_VALUE_WHEN_UNCOMMITTED =
RowVersion::isUncommitted;
- private final int partId;
+ private final int partitionId;
private final int groupId;
- private final VersionChainFreeList versionChainFreeList;
private final VersionChainTree versionChainTree;
- private final VersionChainDataPageReader versionChainDataPageReader;
private final RowVersionFreeList rowVersionFreeList;
private final DataPageReader rowVersionDataPageReader;
- private final ThreadLocal<ReadRowVersion> readRowVersionCache =
ThreadLocal.withInitial(ReadRowVersion::new);
- private final ThreadLocal<ScanVersionChainByTimestamp>
scanVersionChainByTimestampCache = ThreadLocal.withInitial(
- ScanVersionChainByTimestamp::new
- );
+ private final ThreadLocal<ReadRowVersion> readRowVersionCache;
Review Comment:
Does this class really make sense? `ReadRowVersion` is a minuscule class
with a minimum amount of data. Are you sure it would be faster to bother with
`ThreadLocal` instead of creating a new instance?
--
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]
