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/06/17 12:11:57 UTC
[GitHub] [ignite-3] rpuch opened a new pull request, #886: IGNITE-17008 Add support for NextLink field in VersionChain
rpuch opened a new pull request, #886:
URL: https://github.com/apache/ignite-3/pull/886
https://issues.apache.org/jira/browse/IGNITE-17008
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r911834650
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
Review Comment:
The current code in `main` returns `null` if transactionId is `null`, this happens before the assert you mention here, so the assert will not be triggered for 'no write intent' situation. But it probably still makes sense to leave it here (because, if there IS a write intent, it must be uncommitted, i.e. have no timestamp).
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r913442806
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
+ assert latestVersion.isUncommitted();
+
+ removeRowVersion(latestVersion);
- removeRowVersion(currentVersion);
+ if (latestVersion.hasNextLink()) {
+ // This load can be avoided, see the comment below.
+ RowVersion preLatestVersion = readPredecessor(latestVersion);
Review Comment:
Renamed the variable to `latestCommittedVersion`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r911839495
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/VersionChainDataIo.java:
##########
@@ -68,14 +68,16 @@ protected void writeRowData(long pageAddr, int dataOff, int payloadSize, Version
addr += TransactionIds.writeTransactionId(addr, 0, row.transactionId());
addr += PartitionlessLinks.writeToMemory(addr, row.headLink());
+ addr += PartitionlessLinks.writeToMemory(addr, row.nextLink());
}
/** {@inheritDoc} */
@Override
protected void writeFragmentData(VersionChain row, ByteBuffer buf, int rowOff, int payloadSize) {
assertPageType(buf);
- throw new UnsupportedOperationException("Splitting version chain rows to fragments is ridiculous!");
+ throw new IllegalStateException("Version chains must never be split to fragments, this should be guaranteed "
Review Comment:
Agreed, here `UnsupportedOperationException` is better. Fixed
--
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 #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r912725715
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
Review Comment:
Right now I can't navigate to the code from this comments branch, because you force-pushed your code and commits have changed. Why did you do this?
Anyways, I obviously missed the part where you return null, that's on me. Everything's good
--
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 #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r911728747
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
Review Comment:
I believe that this logic is already changed in master, can you check? Aborting a row that had no write intent is not an exceptional situation anymore
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/io/VersionChainDataIo.java:
##########
@@ -68,14 +68,16 @@ protected void writeRowData(long pageAddr, int dataOff, int payloadSize, Version
addr += TransactionIds.writeTransactionId(addr, 0, row.transactionId());
addr += PartitionlessLinks.writeToMemory(addr, row.headLink());
+ addr += PartitionlessLinks.writeToMemory(addr, row.nextLink());
}
/** {@inheritDoc} */
@Override
protected void writeFragmentData(VersionChain row, ByteBuffer buf, int rowOff, int payloadSize) {
assertPageType(buf);
- throw new UnsupportedOperationException("Splitting version chain rows to fragments is ridiculous!");
+ throw new IllegalStateException("Version chains must never be split to fragments, this should be guaranteed "
Review Comment:
I'm confident that "IllegalStateException" is wildly misused in Ignite. UnsupportedOperationException looked more appropriate
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
+ assert latestVersion.isUncommitted();
+
+ removeRowVersion(latestVersion);
- removeRowVersion(currentVersion);
+ if (latestVersion.hasNextLink()) {
+ // This load can be avoided, see the comment below.
+ RowVersion preLatestVersion = readPredecessor(latestVersion);
Review Comment:
Using a word "predecessor" in conjuntion with "next" later in the code is confusing. Can we come up with something better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] rpuch commented on a diff in pull request #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
rpuch commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r911851204
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
+ assert latestVersion.isUncommitted();
+
+ removeRowVersion(latestVersion);
- removeRowVersion(currentVersion);
+ if (latestVersion.hasNextLink()) {
+ // This load can be avoided, see the comment below.
+ RowVersion preLatestVersion = readPredecessor(latestVersion);
Review Comment:
I think the problem is caused by the fact that we have 2 directions. One is chronological (previous is what was created before, next is what was created later), another one is 'chain order' ('next' in the chain is what was created before). This is because elements in our linked list come backwards.
The 'order of chain' is important on a lower level, and on the higher level it seems to be more convenient to use the 'creation order', so I use 'latest' as the term to mean 'the version created most recently'.
The problem is when we go from one terminology ('next' in the 'chain order') to another ('predecessor' in the 'creation order').
I see the following possibilities:
1. Change the terminology on the chain level: 'tail' instead of 'head', 'previous' instead of 'next'. Radical and contradicts with the usual terminology.
2. Keep head/next terminology on the chain level, but on the store level, avoid using 'latest', instead use 'current'. But it's unclear how to deal with the version that was created just before the 'current'. Pre-current or next-to-current?
3. Have a convention that we separate the levels. On the lower (chain) level, we use head/next terminology; on the higher (store) level, we use latest/predecessor terminology. Can still be confusing. This is what I implemented for now as a response to your comment.
WDYT?
--
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 #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #886:
URL: https://github.com/apache/ignite-3/pull/886#discussion_r912735528
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -338,24 +353,38 @@ private VersionChain findVersionChainForModification(RowId rowId) {
throw new NoUncommittedVersionException();
}
- RowVersion currentVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
- assert currentVersion.isUncommitted();
+ RowVersion latestVersion = findLatestRowVersion(currentVersionChain, ALWAYS_LOAD_VALUE);
+ assert latestVersion.isUncommitted();
+
+ removeRowVersion(latestVersion);
- removeRowVersion(currentVersion);
+ if (latestVersion.hasNextLink()) {
+ // This load can be avoided, see the comment below.
+ RowVersion preLatestVersion = readPredecessor(latestVersion);
Review Comment:
So, my thoughts are:
- If something is a "predecessor", you shouldn't call the variable "preSomething", because it's indistinguishable from "previous", which is the opposite of "next", and "next" is from the other naming convention :)
- There's a very simple solution here, just call that variable "latestCommittedVersion" or something (maybe "newest"). It doesn't have to be named in relation to its neighbor.
- I'd prefer having a single convention. There's a chain of versions. It has a head and "next" links, this is fine. Names like "oldest" and "newest" are also fine. Words like "predecessor" / "descendant" or whatever look out of place, even though your argument is correct and there are several perspectives.
--
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 #886: IGNITE-17008 Add support for NextLink field in VersionChain
Posted by GitBox <gi...@apache.org>.
ibessonov merged PR #886:
URL: https://github.com/apache/ignite-3/pull/886
--
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