You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/08/12 08:27:13 UTC
[GitHub] [ignite-3] ibessonov opened a new pull request, #1003: IGNITE-17076 Common RowId implementation introduced
ibessonov opened a new pull request, #1003:
URL: https://github.com/apache/ignite-3/pull/1003
https://issues.apache.org/jira/browse/IGNITE-17076
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945609518
##########
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:
Ok
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945611461
##########
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:
Ok, I guess I have to write them then
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945609336
##########
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:
Oh, didn't know that
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945604660
##########
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:
Ok, just in case if this shows a bug in transaction protocol
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945878802
##########
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:
there might be a desire to add tests to sub-classes, and they may need these methods
--
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] tkalkirill commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945534744
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RowVersion.java:
##########
@@ -36,11 +37,16 @@ public class RowVersion implements Storable {
* A 'timestamp' representing absense of a timestamp.
*/
public static final Timestamp NULL_TIMESTAMP = new Timestamp(Long.MIN_VALUE, Long.MIN_VALUE);
+
/**
* Represents an absent partitionless link.
*/
public static final long NULL_LINK = 0;
+ public static boolean isNullLink(long link) {
+ return PageIdUtils.pageIndex(link) == 0;
Review Comment:
```suggestion
return PageIdUtils.pageIndex(link) == NULL_LINK;
```
--
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] sashapolo commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945844122
##########
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:
maybe we should change it to `short`?
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945548531
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RowVersion.java:
##########
@@ -36,11 +37,16 @@ public class RowVersion implements Storable {
* A 'timestamp' representing absense of a timestamp.
*/
public static final Timestamp NULL_TIMESTAMP = new Timestamp(Long.MIN_VALUE, Long.MIN_VALUE);
+
/**
* Represents an absent partitionless link.
*/
public static final long NULL_LINK = 0;
+ public static boolean isNullLink(long link) {
+ return PageIdUtils.pageIndex(link) == 0;
Review Comment:
I can introduce additional constant if you wish
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945549406
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RowVersion.java:
##########
@@ -36,11 +37,16 @@ public class RowVersion implements Storable {
* A 'timestamp' representing absense of a timestamp.
*/
public static final Timestamp NULL_TIMESTAMP = new Timestamp(Long.MIN_VALUE, Long.MIN_VALUE);
+
/**
* Represents an absent partitionless link.
*/
public static final long NULL_LINK = 0;
+ public static boolean isNullLink(long link) {
+ return PageIdUtils.pageIndex(link) == 0;
Review Comment:
You know what, I think there's a way to simplify the code in some places
--
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] sashapolo commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945873358
##########
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:
ok
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov merged PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945610318
##########
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:
Might be useful if one wants to write a code like `addr += writePartitionlessLink(addr, row.nextLink());`
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945609187
##########
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 don't know
--
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] tkalkirill commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945508665
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -107,7 +114,10 @@ public interface MvPartitionStorage extends AutoCloseable {
* @param txId Transaction id.
* @return Row id.
* @throws StorageException If failed to write data into the storage.
+ *
+ * @deprecated Generates different ids for each replica.
Review Comment:
What to use instead?
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945548344
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RowVersion.java:
##########
@@ -36,11 +37,16 @@ public class RowVersion implements Storable {
* A 'timestamp' representing absense of a timestamp.
*/
public static final Timestamp NULL_TIMESTAMP = new Timestamp(Long.MIN_VALUE, Long.MIN_VALUE);
+
/**
* Represents an absent partitionless link.
*/
public static final long NULL_LINK = 0;
+ public static boolean isNullLink(long link) {
+ return PageIdUtils.pageIndex(link) == 0;
Review Comment:
No, NULL_LINK is a link, but 0 page index is not a link. It's not even a "long" value.
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945626512
##########
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:
No, I'm not sure about it. Allocation is fast enough. Should I get rid of these?
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945625435
##########
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:
I just left it here, should I remove it?
I don't mind passing "null" explicitly tbh
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945607969
##########
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:
They are not for override, but rather for being used in subclasses
--
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] sashapolo commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945843820
##########
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:
why would subclasses need these methods?
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945853634
##########
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:
No, too many small changes
--
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] tkalkirill commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945529418
##########
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. */
Review Comment:
Please describe why **short** and not **int**.
--
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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945530386
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/MvPartitionStorage.java:
##########
@@ -107,7 +114,10 @@ public interface MvPartitionStorage extends AutoCloseable {
* @param txId Transaction id.
* @return Row id.
* @throws StorageException If failed to write data into the storage.
+ *
+ * @deprecated Generates different ids for each replica.
Review Comment:
I'll expand the comment.
--
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] sashapolo commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
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: 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 #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945610941
##########
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:
Correct, too bad there's no such check in Javadoc suite
--
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] sashapolo commented on a diff in pull request #1003: IGNITE-17076 Common RowId implementation introduced
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1003:
URL: https://github.com/apache/ignite-3/pull/1003#discussion_r945844365
##########
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:
ok
--
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