ibessonov commented on code in PR #1673:
URL: https://github.com/apache/ignite-3/pull/1673#discussion_r1109441281
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PartitionMeta.java:
##########
@@ -60,6 +60,8 @@ public class PartitionMeta {
private volatile long indexTreeMetaPageId;
+ private volatile long garbageCollectionTreeMetaPageId;
Review Comment:
We are allowed to use "gc" instead of garbageCollection, ok? And we can call
it a queue btw, makes more sense
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbortWriteInvokeClosure.java:
##########
@@ -74,10 +70,7 @@ public void call(@Nullable VersionChain oldRow) throws
IgniteInternalCheckedExce
toRemove = latestVersion;
if (latestVersion.hasNextLink()) {
- // Next can be safely replaced with any value (like 0), because
this field is only used when there
- // is some uncommitted value, but when we add an uncommitted
value, we 'fix' such placeholder value
- // (like 0) by replacing it with a valid value.
- newRow = VersionChain.createCommitted(rowId,
latestVersion.nextLink(), NULL_LINK);
+ newRow = VersionChain.createCommitted(rowId,
latestVersion.nextLink(), latestVersion.nextLink());
Review Comment:
What's the reason for this change?
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaManagerTest.java:
##########
@@ -138,7 +138,7 @@ void testReadWritePartitionMeta(@WorkDirectory Path
workDir) throws Exception {
try (FilePageStore filePageStore =
createFilePageStore(testFilePath)) {
manager.writeMetaToBuffer(
partId,
- new PartitionMeta(UUID.randomUUID(), 100, 10, 34, 900,
500, 300, 200, 4).metaSnapshot(null),
+ new PartitionMeta(UUID.randomUUID(), 100, 10, 34, 900,
500, 300, 200, 400, 4).metaSnapshot(null),
Review Comment:
I wonder if it's the proper time to introduce builder for the meta class.
This constructor is terrible, I hate it
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/util/PageIdUtils.java:
##########
@@ -182,6 +182,15 @@ public static int partitionId(long pageId) {
return (int) ((pageId >>> PAGE_IDX_SIZE) & PART_ID_MASK);
}
+ /**
+ * Extracts partition ID from the page link.
+ *
+ * @param link Page link.
+ */
+ public static int partitionIdFromLink(long link) {
+ return partitionId(pageId(link));
Review Comment:
What's the point of converting link to pageId here? Those bytes will be
ignored anyway
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -373,7 +383,11 @@ ReadResult findLatestRowVersion(VersionChain versionChain)
{
}
RowVersion readRowVersion(long rowVersionLink, Predicate<HybridTimestamp>
loadValue) {
- ReadRowVersion read = new ReadRowVersion(partitionId);
+ return readRowVersion(rowVersionLink, loadValue, true);
+ }
Review Comment:
Ok, I don't get it. We have a predicate that says whether we should or
should not load value. Why did you add another flag? What does it mean?
Not obvious at all, at least it's hard for me to understand it off the bat.
You can document parameters, for example, would be nice
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -922,4 +960,58 @@ protected <T> T inUpdateVersionChainLock(RowId rowId,
Supplier<T> supplier) {
});
}
}
+
+ @Override
+ public @Nullable BinaryRowAndRowId pollForVacuum(HybridTimestamp
lowWatermark) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ GarbageCollectionRowVersion first = gcQueue.findFirstFromGc();
+
+ // Garbage collection queue is empty.
+ if (first == null) {
+ return null;
+ }
+
+ HybridTimestamp rowTimestamp = first.getTimestamp();
+
+ // There are no versions in the garbage collection queue before
watermark.
+ if (rowTimestamp.compareTo(lowWatermark) > 0) {
+ return null;
+ }
+
+ RowId rowId = first.getRowId();
+
+ return inUpdateVersionChainLock(rowId, () -> {
+ // Someone processed the element in parallel.
+ if (!gcQueue.removeFromGc(rowId, rowTimestamp)) {
Review Comment:
Same here
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -922,4 +960,58 @@ protected <T> T inUpdateVersionChainLock(RowId rowId,
Supplier<T> supplier) {
});
}
}
+
+ @Override
+ public @Nullable BinaryRowAndRowId pollForVacuum(HybridTimestamp
lowWatermark) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ GarbageCollectionRowVersion first = gcQueue.findFirstFromGc();
+
+ // Garbage collection queue is empty.
+ if (first == null) {
+ return null;
+ }
+
+ HybridTimestamp rowTimestamp = first.getTimestamp();
+
+ // There are no versions in the garbage collection queue before
watermark.
+ if (rowTimestamp.compareTo(lowWatermark) > 0) {
+ return null;
+ }
+
+ RowId rowId = first.getRowId();
+
+ return inUpdateVersionChainLock(rowId, () -> {
+ // Someone processed the element in parallel.
+ if (!gcQueue.removeFromGc(rowId, rowTimestamp)) {
+ return null;
+ }
+
+ RowVersion removedRowVersion = removeWriteOnGc(rowId,
rowTimestamp);
+
+ return new
BinaryRowAndRowId(rowVersionToBinaryRow(removedRowVersion), rowId);
+ });
+ });
+ }
+
+ private RowVersion removeWriteOnGc(RowId rowId, HybridTimestamp
rowTimestamp) {
+ RemoveWriteOnGcInvokeClosure removeWriteOnGc = new
RemoveWriteOnGcInvokeClosure(rowId, rowTimestamp, this);
Review Comment:
Timestamp, huh. So you scan the entire chain when you need to delete a
single version, am I correct?
I don't like this approach, it's wasteful. You could have a direct link to
the version and just delete the next element of it. Like it's done in the test
implementation, you know
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -922,4 +960,58 @@ protected <T> T inUpdateVersionChainLock(RowId rowId,
Supplier<T> supplier) {
});
}
}
+
+ @Override
+ public @Nullable BinaryRowAndRowId pollForVacuum(HybridTimestamp
lowWatermark) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ GarbageCollectionRowVersion first = gcQueue.findFirstFromGc();
+
+ // Garbage collection queue is empty.
+ if (first == null) {
+ return null;
+ }
+
+ HybridTimestamp rowTimestamp = first.getTimestamp();
+
+ // There are no versions in the garbage collection queue before
watermark.
+ if (rowTimestamp.compareTo(lowWatermark) > 0) {
+ return null;
+ }
+
+ RowId rowId = first.getRowId();
+
+ return inUpdateVersionChainLock(rowId, () -> {
+ // Someone processed the element in parallel.
+ if (!gcQueue.removeFromGc(rowId, rowTimestamp)) {
+ return null;
Review Comment:
So, in case of concurrent calls, you would return null instead of returning
the next entry. We clearly lack such test, please write it and fix the bug, ok?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -922,4 +960,58 @@ protected <T> T inUpdateVersionChainLock(RowId rowId,
Supplier<T> supplier) {
});
}
}
+
+ @Override
+ public @Nullable BinaryRowAndRowId pollForVacuum(HybridTimestamp
lowWatermark) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ GarbageCollectionRowVersion first = gcQueue.findFirstFromGc();
Review Comment:
Why do you have "fromGc" part in the name? It's like having a method
"getFromMap" in "Map" interface, it's redundant
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteCommittedInvokeClosure.java:
##########
@@ -72,23 +74,42 @@ public void call(@Nullable VersionChain oldRow) throws
IgniteInternalCheckedExce
throw new StorageException("Write intent exists: [rowId={}, {}]",
oldRow.rowId(), storage.createStorageInfo());
}
- long nextLink = oldRow == null ? NULL_LINK :
oldRow.newestCommittedLink();
+ if (oldRow == null) {
+ operationType = OperationType.PUT;
+
+ RowVersion newVersion = insertCommittedRowVersion(row,
commitTimestamp, NULL_LINK);
+
+ newRow = VersionChain.createCommitted(rowId, newVersion.link(),
newVersion.nextLink());
+ } else {
+ RowVersion current = storage.readRowVersion(oldRow.headLink(),
ALWAYS_LOAD_VALUE, false);
Review Comment:
Always load value, you say?
First of all, why do you need that?
Second, addWriteCommitted has very specific restrictions on its usages. For
example, paired tombstones shouls already be pre-filtered before insertion!
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java:
##########
@@ -61,11 +68,23 @@ public void call(@Nullable VersionChain oldRow) throws
IgniteInternalCheckedExce
return;
}
- updateTimestampLink = oldRow.headLink();
-
operationType = OperationType.PUT;
- newRow = VersionChain.createCommitted(oldRow.rowId(),
oldRow.headLink(), oldRow.nextLink());
+ RowVersion current = storage.readRowVersion(oldRow.headLink(),
ALWAYS_LOAD_VALUE, false);
+ RowVersion next = oldRow.hasNextLink() ?
storage.readRowVersion(oldRow.nextLink(), ALWAYS_LOAD_VALUE, false) : null;
+
+ // If the previous and current version are tombstones, then delete the
current version.
+ if (next != null && current.isTombstone() && next.isTombstone()) {
Review Comment:
So, I guess you always load value to check if it's a tombstone or not. If
this is correct, then it's like writing "size() == 0" instead of "isEmpty()",
makes no sense. If this is not correct, then I apologize
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java:
##########
@@ -98,4 +117,19 @@ public void onUpdate() {
}
}
}
+
+ /**
+ * Method to call after {@link BplusTree#invoke(Object, Object,
InvokeClosure)} has completed.
+ */
+ void afterCompletion() {
+ assert operationType == OperationType.PUT ? true : toRemove == null :
"toRemove=" + toRemove + ", op=" + operationType;
Review Comment:
Same here
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java:
##########
@@ -84,10 +103,10 @@ public OperationType operationType() {
@Override
public void onUpdate() {
- assert operationType == OperationType.PUT ? updateTimestampLink !=
null : updateTimestampLink == null :
+ assert operationType == OperationType.PUT ? true : updateTimestampLink
== NULL_LINK :
Review Comment:
Oh my god!
You know that `a ? true : b` is the same as `a || b`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/CommitWriteInvokeClosure.java:
##########
@@ -61,11 +68,23 @@ public void call(@Nullable VersionChain oldRow) throws
IgniteInternalCheckedExce
return;
}
- updateTimestampLink = oldRow.headLink();
-
operationType = OperationType.PUT;
- newRow = VersionChain.createCommitted(oldRow.rowId(),
oldRow.headLink(), oldRow.nextLink());
+ RowVersion current = storage.readRowVersion(oldRow.headLink(),
ALWAYS_LOAD_VALUE, false);
+ RowVersion next = oldRow.hasNextLink() ?
storage.readRowVersion(oldRow.nextLink(), ALWAYS_LOAD_VALUE, false) : null;
+
+ // If the previous and current version are tombstones, then delete the
current version.
+ if (next != null && current.isTombstone() && next.isTombstone()) {
+ toRemove = current;
+
+ newRow = VersionChain.createCommitted(oldRow.rowId(), next.link(),
next.nextLink());
Review Comment:
Shouldn't newRow just match the oldRow, and the operation would be NOOP? Why
would you need to PUT the same value?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RemoveWriteOnGcInvokeClosure.java:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.pagememory.mv;
+
+import static
org.apache.ignite.internal.storage.pagememory.mv.AbstractPageMemoryMvPartitionStorage.ALWAYS_LOAD_VALUE;
+import static
org.apache.ignite.internal.storage.pagememory.mv.FindRowVersion.RowVersionFilter.equalsByNextLink;
+import static
org.apache.ignite.internal.storage.pagememory.mv.FindRowVersion.RowVersionFilter.equalsByTimestamp;
+
+import java.util.List;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.pagememory.tree.IgniteTree.InvokeClosure;
+import org.apache.ignite.internal.pagememory.tree.IgniteTree.OperationType;
+import org.apache.ignite.internal.storage.RowId;
+import org.apache.ignite.internal.storage.StorageException;
+import org.apache.ignite.lang.IgniteInternalCheckedException;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Implementation of {@link InvokeClosure} for deleting a row version in
version chain on garbage collection in
+ * {@link AbstractPageMemoryMvPartitionStorage#pollForVacuum(HybridTimestamp)}.
+ *
+ * <p>See {@link AbstractPageMemoryMvPartitionStorage} about synchronization.
+ *
+ * <p>Operation may throw {@link StorageException} which will cause form
{@link BplusTree#invoke(Object, Object, InvokeClosure)}.
+ */
+public class RemoveWriteOnGcInvokeClosure implements
InvokeClosure<VersionChain> {
+ private final RowId rowId;
+
+ private final HybridTimestamp timestamp;
+
+ private final AbstractPageMemoryMvPartitionStorage storage;
+
+ private OperationType operationType;
+
+ private @Nullable VersionChain newRow;
+
+ private List<RowVersion> toRemove;
+
+ private RowVersion result;
+
+ private @Nullable RowVersion toUpdate;
+
+ RemoveWriteOnGcInvokeClosure(RowId rowId, HybridTimestamp timestamp,
AbstractPageMemoryMvPartitionStorage storage) {
+ this.rowId = rowId;
+ this.timestamp = timestamp;
+ this.storage = storage;
+ }
+
+ @Override
+ public void call(@Nullable VersionChain oldRow) throws
IgniteInternalCheckedException {
+ assert oldRow != null : "rowId=" + rowId + ", storage=" +
storage.createStorageInfo();
+ assert oldRow.hasNextLink() : oldRow;
+
+ RowVersion rowVersion = findRowVersionLinkWithChecks(oldRow);
+ RowVersion nextRowVersion =
storage.readRowVersion(rowVersion.nextLink(), ALWAYS_LOAD_VALUE, true);
+
+ result = nextRowVersion;
+
+ // If the found version is a tombstone, then we must remove it as well.
+ if (rowVersion.isTombstone()) {
+ toRemove = List.of(nextRowVersion, rowVersion);
+
+ // If the found version is the head of the chain, then delete the
entire chain.
+ if (oldRow.headLink() == rowVersion.link()) {
+ operationType = OperationType.REMOVE;
+ } else if (oldRow.nextLink() == rowVersion.link()) {
+ operationType = OperationType.PUT;
+
+ // Find the version for which this version is
RowVersion#nextLink.
+ toUpdate = storage.readRowVersion(oldRow.headLink(),
ALWAYS_LOAD_VALUE, false);
+
+ newRow = oldRow.setNextLink(nextRowVersion.nextLink());
+ } else {
+ operationType = OperationType.PUT;
+
+ newRow = oldRow;
+
+ // Find the version for which this version is
RowVersion#nextLink.
+ toUpdate = storage.foundRowVersion(oldRow,
equalsByNextLink(rowVersion.link()), false);
+ }
+ } else {
+ operationType = OperationType.PUT;
+
+ toRemove = List.of(nextRowVersion);
+
+ toUpdate = rowVersion;
+
+ if (oldRow.headLink() == rowVersion.link()) {
+ newRow = oldRow.setNextLink(nextRowVersion.nextLink());
+ } else {
+ newRow = oldRow;
+ }
+ }
+ }
+
+ @Override
+ public @Nullable VersionChain newRow() {
+ assert operationType == OperationType.PUT ? newRow != null : newRow ==
null : "newRow=" + newRow + ", op=" + operationType;
+
+ return newRow;
+ }
+
+ @Override
+ public OperationType operationType() {
+ assert operationType != null;
+
+ return operationType;
+ }
+
+ @Override
+ public void onUpdate() {
+ if (toUpdate != null) {
+ try {
+ storage.rowVersionFreeList.updateNextLink(toUpdate.link(),
result.nextLink());
Review Comment:
Do you really have to do this while holding a write lock on a tree page?
This is interesting actually. Technically, no RO transaction should attempt
reads below LW, so entry that's being deleted should never be read. Please
think carefully about this place.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RemoveWriteOnGcInvokeClosure.java:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.pagememory.mv;
+
+import static
org.apache.ignite.internal.storage.pagememory.mv.AbstractPageMemoryMvPartitionStorage.ALWAYS_LOAD_VALUE;
+import static
org.apache.ignite.internal.storage.pagememory.mv.FindRowVersion.RowVersionFilter.equalsByNextLink;
+import static
org.apache.ignite.internal.storage.pagememory.mv.FindRowVersion.RowVersionFilter.equalsByTimestamp;
+
+import java.util.List;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.pagememory.tree.IgniteTree.InvokeClosure;
+import org.apache.ignite.internal.pagememory.tree.IgniteTree.OperationType;
+import org.apache.ignite.internal.storage.RowId;
+import org.apache.ignite.internal.storage.StorageException;
+import org.apache.ignite.lang.IgniteInternalCheckedException;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Implementation of {@link InvokeClosure} for deleting a row version in
version chain on garbage collection in
+ * {@link AbstractPageMemoryMvPartitionStorage#pollForVacuum(HybridTimestamp)}.
+ *
+ * <p>See {@link AbstractPageMemoryMvPartitionStorage} about synchronization.
+ *
+ * <p>Operation may throw {@link StorageException} which will cause form
{@link BplusTree#invoke(Object, Object, InvokeClosure)}.
+ */
+public class RemoveWriteOnGcInvokeClosure implements
InvokeClosure<VersionChain> {
+ private final RowId rowId;
+
+ private final HybridTimestamp timestamp;
+
+ private final AbstractPageMemoryMvPartitionStorage storage;
+
+ private OperationType operationType;
+
+ private @Nullable VersionChain newRow;
+
+ private List<RowVersion> toRemove;
+
+ private RowVersion result;
+
+ private @Nullable RowVersion toUpdate;
+
+ RemoveWriteOnGcInvokeClosure(RowId rowId, HybridTimestamp timestamp,
AbstractPageMemoryMvPartitionStorage storage) {
+ this.rowId = rowId;
+ this.timestamp = timestamp;
+ this.storage = storage;
+ }
+
+ @Override
+ public void call(@Nullable VersionChain oldRow) throws
IgniteInternalCheckedException {
+ assert oldRow != null : "rowId=" + rowId + ", storage=" +
storage.createStorageInfo();
+ assert oldRow.hasNextLink() : oldRow;
+
+ RowVersion rowVersion = findRowVersionLinkWithChecks(oldRow);
+ RowVersion nextRowVersion =
storage.readRowVersion(rowVersion.nextLink(), ALWAYS_LOAD_VALUE, true);
+
+ result = nextRowVersion;
+
+ // If the found version is a tombstone, then we must remove it as well.
+ if (rowVersion.isTombstone()) {
+ toRemove = List.of(nextRowVersion, rowVersion);
+
+ // If the found version is the head of the chain, then delete the
entire chain.
+ if (oldRow.headLink() == rowVersion.link()) {
+ operationType = OperationType.REMOVE;
+ } else if (oldRow.nextLink() == rowVersion.link()) {
+ operationType = OperationType.PUT;
Review Comment:
Why does it have to be PUT, it's a NOOP operation, right?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RowVersion.java:
##########
@@ -112,7 +124,7 @@ boolean isTombstone() {
}
static boolean isTombstone(int valueSize) {
- return valueSize == 0;
+ return valueSize <= 0;
Review Comment:
How is it possible that the size is negative? It's not mentioned in
documentation for this class, or any other as far as I see
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VersionChain.java:
##########
@@ -131,7 +131,29 @@ public boolean hasCommittedVersions() {
return newestCommittedLink() != NULL_LINK;
}
- /** {@inheritDoc} */
+ /**
+ * Returns {@code true} if there is a link to the next version.
+ */
+ public boolean hasNextLink() {
+ return nextLink != NULL_LINK;
+ }
+
+ /**
+ * Returns {@code true} if there is a link to the head version.
+ */
+ public boolean hasHeadLink() {
+ return headLink != NULL_LINK;
+ }
+
+ /**
+ * Creates a copy of the version chain with new next link.
+ *
+ * @param nextLink New next link.
+ */
+ public VersionChain setNextLink(long nextLink) {
Review Comment:
Maybe you should rename it to "withNextLink", "set" is usually mutating
given object instead of creating a new one
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/FindRowVersion.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.pagememory.mv;
+
+import static org.apache.ignite.internal.pagememory.util.PageIdUtils.NULL_LINK;
+import static
org.apache.ignite.internal.pagememory.util.PageIdUtils.partitionIdFromLink;
+import static
org.apache.ignite.internal.pagememory.util.PartitionlessLinks.readPartitionless;
+
+import java.nio.ByteBuffer;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.internal.pagememory.datapage.PageMemoryTraversal;
+import org.apache.ignite.internal.pagememory.io.DataPagePayload;
+import org.apache.ignite.internal.pagememory.util.PageUtils;
+import org.apache.ignite.internal.schema.ByteBufferRow;
+import
org.apache.ignite.internal.storage.pagememory.mv.FindRowVersion.RowVersionFilter;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Search for a row version in version chains.
+ */
+class FindRowVersion implements PageMemoryTraversal<RowVersionFilter> {
+ private final int partitionId;
+
+ private final boolean loadValueBytes;
+
+ private boolean rowVersionFounded;
Review Comment:
"found", not "founded" :(
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/gc/io/GarbageCollectionInnerIo.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.pagememory.mv.gc.io;
+
+import org.apache.ignite.internal.pagememory.io.IoVersions;
+import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.pagememory.tree.io.BplusInnerIo;
+import org.apache.ignite.internal.pagememory.tree.io.BplusIo;
+import
org.apache.ignite.internal.storage.pagememory.mv.gc.GarbageCollectionRowVersion;
+import
org.apache.ignite.internal.storage.pagememory.mv.gc.GarbageCollectionTree;
+
+/**
+ * IO routines for {@link GarbageCollectionTree} inner pages.
+ */
+public class GarbageCollectionInnerIo extends
BplusInnerIo<GarbageCollectionRowVersion> implements GarbageCollectionIo {
+ /** Page IO type. */
+ public static final short T_GARBAGE_COLLECTION_INNER_IO = 15;
Review Comment:
Can we have such constants in separate classes, like in `IndexPageTypes`,
for example?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/gc/io/GarbageCollectionInnerIo.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.storage.pagememory.mv.gc.io;
+
+import org.apache.ignite.internal.pagememory.io.IoVersions;
+import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.pagememory.tree.io.BplusInnerIo;
+import org.apache.ignite.internal.pagememory.tree.io.BplusIo;
+import
org.apache.ignite.internal.storage.pagememory.mv.gc.GarbageCollectionRowVersion;
+import
org.apache.ignite.internal.storage.pagememory.mv.gc.GarbageCollectionTree;
+
+/**
+ * IO routines for {@link GarbageCollectionTree} inner pages.
+ */
+public class GarbageCollectionInnerIo extends
BplusInnerIo<GarbageCollectionRowVersion> implements GarbageCollectionIo {
+ /** Page IO type. */
+ public static final short T_GARBAGE_COLLECTION_INNER_IO = 15;
+
+ /** I/O versions. */
+ public static final IoVersions<GarbageCollectionInnerIo> VERSIONS = new
IoVersions<>(new GarbageCollectionInnerIo(1));
Review Comment:
Please shorten all `GarbageCollection` names, it's too mouthful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
