ibessonov commented on code in PR #3615:
URL: https://github.com/apache/ignite-3/pull/3615#discussion_r1576172447
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/Storable.java:
##########
@@ -59,5 +67,27 @@ public interface Storable {
/**
* Returns I/O for handling this storable.
*/
- IoVersions<? extends AbstractDataPageIo<?>> ioVersions();
+ IoVersions<DataPageIo> ioVersions();
+
+ /** Returns a byte buffer that contains binary tuple data. */
Review Comment:
Excuse me, what binary tuple? It's an abstraction, it should not depend on
the implementation. There's clearly something wrong here.
What was wrong with `writeRowData` and `writeFragmentData` that you could
have moved here?
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeList.java:
##########
@@ -21,30 +21,27 @@
import org.apache.ignite.internal.lang.IgniteInternalCheckedException;
import org.apache.ignite.internal.logger.IgniteLogger;
import org.apache.ignite.internal.pagememory.Storable;
-import org.apache.ignite.internal.pagememory.metric.IoStatisticsHolder;
import org.apache.ignite.internal.pagememory.util.PageHandler;
/**
* Free list.
*/
-public interface FreeList<T extends Storable> {
+public interface FreeList {
/**
* Inserts a row.
*
* @param row Row.
- * @param statHolder Statistics holder to track IO operations.
* @throws IgniteInternalCheckedException If failed.
*/
- void insertDataRow(T row, IoStatisticsHolder statHolder) throws
IgniteInternalCheckedException;
+ void insertDataRow(Storable row) throws IgniteInternalCheckedException;
Review Comment:
Why did you remove stat holder? Let's leave it for now. If you want to clean
the code, then please do it in a separate PR, this one is already big enough.
##########
modules/core/src/main/java/org/apache/ignite/internal/util/CachedIterator.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.util;
+
+import java.util.Iterator;
+
+/** {@link Iterator} implementation that allows to access the current element
multiple times. */
+public class CachedIterator<T> implements Iterator<T> {
Review Comment:
Maybe we shouldn't put it in `core`, this class is very specific. `get`
contract is weird and hard to explain, so I'd rather hide it
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java:
##########
@@ -886,9 +614,28 @@ public long recycledPagesCount() throws
IgniteInternalCheckedException {
}
}
- /** {@inheritDoc} */
+ @Override
+ public void saveMetadata() throws IgniteInternalCheckedException {
+ saveMetadata(statHolder);
+ }
+
+ /** Returns page eviction tracker. */
+ public PageEvictionTracker evictionTracker() {
+ return evictionTracker;
+ }
+
+ /** Returns page memory. */
+ public PageMemory pageMemory() {
+ return pageMem;
+ }
+
+ /** Returns write row handler. */
+ public WriteRowHandler writeRowHandler() {
Review Comment:
This is weird, why did you make it a method? Artifact of the refactoring?
I would recommend you to rollback some of the changes.
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java:
##########
@@ -84,300 +86,59 @@ public abstract class AbstractFreeList<T extends Storable>
extends PagesList imp
protected final PageEvictionTracker evictionTracker;
/** Write a single row on a single page. */
- private final WriteRowHandler writeRowHnd = new WriteRowHandler();
+ private final WriteRowHandler writeRowHnd = new WriteRowHandler(this);
/** Write multiple rows on a single page. */
- private final WriteRowsHandler writeRowsHnd = new WriteRowsHandler();
+ private final WriteRowsHandler writeRowsHnd = new WriteRowsHandler(this);
private final PageHandler<ReuseBag, Long> rmvRow;
- private class WriteRowHandler implements PageHandler<T, Integer> {
- /** {@inheritDoc} */
- @Override
- public Integer run(
- int cacheId,
- long pageId,
- long page,
- long pageAddr,
- PageIo iox,
- T row,
- int written,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- written = addRow(pageId, pageAddr, iox, row, written);
-
- putPage(((AbstractDataPageIo<Storable>)
iox).getFreeSpace(pageAddr), pageId, pageAddr, statHolder);
-
- return written;
- }
-
- /**
- * Writes row to data page.
- *
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param iox IO.
- * @param row Row to write.
- * @param written Written size.
- * @return Number of bytes written, {@link #COMPLETE} if the row was
fully written.
- * @throws IgniteInternalCheckedException If failed.
- */
- protected Integer addRow(
- long pageId,
- long pageAddr,
- PageIo iox,
- T row,
- int written
- ) throws IgniteInternalCheckedException {
- AbstractDataPageIo<T> io = (AbstractDataPageIo<T>) iox;
-
- int rowSize = row.size();
- int oldFreeSpace = io.getFreeSpace(pageAddr);
-
- assert oldFreeSpace > 0 : oldFreeSpace;
-
- // If the full row does not fit into this page write only a
fragment.
- written = (written == 0 && oldFreeSpace >= rowSize) ?
addRowFull(pageId, pageAddr, io, row, rowSize) :
- addRowFragment(pageId, pageAddr, io, row, written,
rowSize);
-
- if (written == rowSize) {
- evictionTracker.touchPage(pageId);
- }
-
- // Avoid boxing with garbage generation for usual case.
- return written == rowSize ? COMPLETE : written;
- }
-
- /**
- * Adds row to this data page and sets respective link to the given
row object.
- *
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param io IO.
- * @param row Row.
- * @param rowSize Row size.
- * @return Written size which is always equal to row size here.
- * @throws IgniteInternalCheckedException If failed.
- */
- protected int addRowFull(
- long pageId,
- long pageAddr,
- AbstractDataPageIo<T> io,
- T row,
- int rowSize
- ) throws IgniteInternalCheckedException {
- io.addRow(pageId, pageAddr, row, rowSize, pageSize());
-
- return rowSize;
- }
-
- /**
- * Adds maximum possible fragment of the given row to this data page
and sets respective link to the row.
- *
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param io IO.
- * @param row Row.
- * @param written Written size.
- * @param rowSize Row size.
- * @return Updated written size.
- * @throws IgniteInternalCheckedException If failed.
- */
- protected int addRowFragment(
- long pageId,
- long pageAddr,
- AbstractDataPageIo<T> io,
- T row,
- int written,
- int rowSize
- ) throws IgniteInternalCheckedException {
- int payloadSize = io.addRowFragment(pageMem, pageId, pageAddr,
row, written, rowSize, pageSize());
-
- assert payloadSize > 0 : payloadSize;
-
- return written + payloadSize;
- }
-
- /**
- * Put page into the free list if needed.
- *
- * @param freeSpace Page free space.
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param statHolder Statistics holder to track IO operations.
- */
- protected void putPage(
- int freeSpace,
- long pageId,
- long pageAddr,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- if (freeSpace > MIN_PAGE_FREE_SPACE) {
- int bucket = bucket(freeSpace, false);
-
- put(null, pageId, pageAddr, bucket, statHolder);
- }
- }
- }
-
- private final class WriteRowsHandler implements
PageHandler<CachedIterator<T>, Integer> {
- /** {@inheritDoc} */
- @Override
- public Integer run(
- int cacheId,
- long pageId,
- long page,
- long pageAddr,
- PageIo iox,
- CachedIterator<T> it,
- int written,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- AbstractDataPageIo<T> io = (AbstractDataPageIo<T>) iox;
-
- // Fill the page up to the end.
- while (written != COMPLETE || (!evictionTracker.evictionRequired()
&& it.hasNext())) {
- T row = it.get();
-
- if (written == COMPLETE) {
- row = it.next();
-
- // If the data row was completely written without
remainder, proceed to the next.
- if ((written = writeWholePages(row, statHolder)) ==
COMPLETE) {
- continue;
- }
-
- if (io.getFreeSpace(pageAddr) < row.size() - written) {
- break;
- }
- }
-
- written = writeRowHnd.addRow(pageId, pageAddr, io, row,
written);
-
- assert written == COMPLETE;
-
- evictionTracker.touchPage(pageId);
- }
-
- writeRowHnd.putPage(io.getFreeSpace(pageAddr), pageId, pageAddr,
statHolder);
-
- return written;
- }
- }
-
- private final class RemoveRowHandler implements PageHandler<ReuseBag,
Long> {
- /** Indicates whether partition ID should be masked from page ID. */
- private final boolean maskPartId;
-
- /**
- * Constructor.
- *
- * @param maskPartId Indicates whether partition ID should be masked
from page ID.
- */
- RemoveRowHandler(boolean maskPartId) {
- this.maskPartId = maskPartId;
- }
-
- /** {@inheritDoc} */
- @Override
- public Long run(
- int cacheId,
- long pageId,
- long page,
- long pageAddr,
- PageIo iox,
- ReuseBag reuseBag,
- int itemId,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- AbstractDataPageIo<T> io = (AbstractDataPageIo<T>) iox;
-
- int oldFreeSpace = io.getFreeSpace(pageAddr);
-
- assert oldFreeSpace >= 0 : oldFreeSpace;
-
- long nextLink = io.removeRow(pageAddr, itemId, pageSize());
-
- int newFreeSpace = io.getFreeSpace(pageAddr);
-
- if (newFreeSpace > MIN_PAGE_FREE_SPACE) {
- int newBucket = bucket(newFreeSpace, false);
-
- boolean putIsNeeded = oldFreeSpace <= MIN_PAGE_FREE_SPACE;
-
- if (!putIsNeeded) {
- int oldBucket = bucket(oldFreeSpace, false);
-
- if (oldBucket != newBucket) {
- // It is possible that page was concurrently taken for
put, in this case put will handle bucket change.
- pageId = maskPartId ?
PageIdUtils.maskPartitionId(pageId) : pageId;
-
- putIsNeeded = removeDataPage(pageId, pageAddr, io,
oldBucket, statHolder);
- }
- }
-
- if (io.isEmpty(pageAddr)) {
- evictionTracker.forgetPage(pageId);
-
- if (putIsNeeded) {
- reuseBag.addFreePage(recyclePage(pageId, pageAddr));
- }
- } else if (putIsNeeded) {
- put(null, pageId, pageAddr, newBucket, statHolder);
- }
- }
-
- // For common case boxed 0L will be cached inside of Long, so no
garbage will be produced.
- return nextLink;
- }
- }
+ private final IoStatisticsHolder statHolder;
Review Comment:
When I asked about static classes, I didn't mean moving them into other
files. And the goal was to make fields static as well. If fields are not static
then what's the point? I think I should have expressed myself more clear last
time.
Also, please ask me if you're not sure what I'm expecting. What you did
right now it pretty much useless and it bloats PR for no good reason :(
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/Storable.java:
##########
@@ -59,5 +67,27 @@ public interface Storable {
/**
* Returns I/O for handling this storable.
*/
- IoVersions<? extends AbstractDataPageIo<?>> ioVersions();
+ IoVersions<DataPageIo> ioVersions();
Review Comment:
This method is useless now
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java:
##########
@@ -84,300 +86,59 @@ public abstract class AbstractFreeList<T extends Storable>
extends PagesList imp
protected final PageEvictionTracker evictionTracker;
/** Write a single row on a single page. */
- private final WriteRowHandler writeRowHnd = new WriteRowHandler();
+ private final WriteRowHandler writeRowHnd = new WriteRowHandler(this);
/** Write multiple rows on a single page. */
- private final WriteRowsHandler writeRowsHnd = new WriteRowsHandler();
+ private final WriteRowsHandler writeRowsHnd = new WriteRowsHandler(this);
private final PageHandler<ReuseBag, Long> rmvRow;
- private class WriteRowHandler implements PageHandler<T, Integer> {
- /** {@inheritDoc} */
- @Override
- public Integer run(
- int cacheId,
- long pageId,
- long page,
- long pageAddr,
- PageIo iox,
- T row,
- int written,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- written = addRow(pageId, pageAddr, iox, row, written);
-
- putPage(((AbstractDataPageIo<Storable>)
iox).getFreeSpace(pageAddr), pageId, pageAddr, statHolder);
-
- return written;
- }
-
- /**
- * Writes row to data page.
- *
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param iox IO.
- * @param row Row to write.
- * @param written Written size.
- * @return Number of bytes written, {@link #COMPLETE} if the row was
fully written.
- * @throws IgniteInternalCheckedException If failed.
- */
- protected Integer addRow(
- long pageId,
- long pageAddr,
- PageIo iox,
- T row,
- int written
- ) throws IgniteInternalCheckedException {
- AbstractDataPageIo<T> io = (AbstractDataPageIo<T>) iox;
-
- int rowSize = row.size();
- int oldFreeSpace = io.getFreeSpace(pageAddr);
-
- assert oldFreeSpace > 0 : oldFreeSpace;
-
- // If the full row does not fit into this page write only a
fragment.
- written = (written == 0 && oldFreeSpace >= rowSize) ?
addRowFull(pageId, pageAddr, io, row, rowSize) :
- addRowFragment(pageId, pageAddr, io, row, written,
rowSize);
-
- if (written == rowSize) {
- evictionTracker.touchPage(pageId);
- }
-
- // Avoid boxing with garbage generation for usual case.
- return written == rowSize ? COMPLETE : written;
- }
-
- /**
- * Adds row to this data page and sets respective link to the given
row object.
- *
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param io IO.
- * @param row Row.
- * @param rowSize Row size.
- * @return Written size which is always equal to row size here.
- * @throws IgniteInternalCheckedException If failed.
- */
- protected int addRowFull(
- long pageId,
- long pageAddr,
- AbstractDataPageIo<T> io,
- T row,
- int rowSize
- ) throws IgniteInternalCheckedException {
- io.addRow(pageId, pageAddr, row, rowSize, pageSize());
-
- return rowSize;
- }
-
- /**
- * Adds maximum possible fragment of the given row to this data page
and sets respective link to the row.
- *
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param io IO.
- * @param row Row.
- * @param written Written size.
- * @param rowSize Row size.
- * @return Updated written size.
- * @throws IgniteInternalCheckedException If failed.
- */
- protected int addRowFragment(
- long pageId,
- long pageAddr,
- AbstractDataPageIo<T> io,
- T row,
- int written,
- int rowSize
- ) throws IgniteInternalCheckedException {
- int payloadSize = io.addRowFragment(pageMem, pageId, pageAddr,
row, written, rowSize, pageSize());
-
- assert payloadSize > 0 : payloadSize;
-
- return written + payloadSize;
- }
-
- /**
- * Put page into the free list if needed.
- *
- * @param freeSpace Page free space.
- * @param pageId Page ID.
- * @param pageAddr Page address.
- * @param statHolder Statistics holder to track IO operations.
- */
- protected void putPage(
- int freeSpace,
- long pageId,
- long pageAddr,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- if (freeSpace > MIN_PAGE_FREE_SPACE) {
- int bucket = bucket(freeSpace, false);
-
- put(null, pageId, pageAddr, bucket, statHolder);
- }
- }
- }
-
- private final class WriteRowsHandler implements
PageHandler<CachedIterator<T>, Integer> {
- /** {@inheritDoc} */
- @Override
- public Integer run(
- int cacheId,
- long pageId,
- long page,
- long pageAddr,
- PageIo iox,
- CachedIterator<T> it,
- int written,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- AbstractDataPageIo<T> io = (AbstractDataPageIo<T>) iox;
-
- // Fill the page up to the end.
- while (written != COMPLETE || (!evictionTracker.evictionRequired()
&& it.hasNext())) {
- T row = it.get();
-
- if (written == COMPLETE) {
- row = it.next();
-
- // If the data row was completely written without
remainder, proceed to the next.
- if ((written = writeWholePages(row, statHolder)) ==
COMPLETE) {
- continue;
- }
-
- if (io.getFreeSpace(pageAddr) < row.size() - written) {
- break;
- }
- }
-
- written = writeRowHnd.addRow(pageId, pageAddr, io, row,
written);
-
- assert written == COMPLETE;
-
- evictionTracker.touchPage(pageId);
- }
-
- writeRowHnd.putPage(io.getFreeSpace(pageAddr), pageId, pageAddr,
statHolder);
-
- return written;
- }
- }
-
- private final class RemoveRowHandler implements PageHandler<ReuseBag,
Long> {
- /** Indicates whether partition ID should be masked from page ID. */
- private final boolean maskPartId;
-
- /**
- * Constructor.
- *
- * @param maskPartId Indicates whether partition ID should be masked
from page ID.
- */
- RemoveRowHandler(boolean maskPartId) {
- this.maskPartId = maskPartId;
- }
-
- /** {@inheritDoc} */
- @Override
- public Long run(
- int cacheId,
- long pageId,
- long page,
- long pageAddr,
- PageIo iox,
- ReuseBag reuseBag,
- int itemId,
- IoStatisticsHolder statHolder
- ) throws IgniteInternalCheckedException {
- AbstractDataPageIo<T> io = (AbstractDataPageIo<T>) iox;
-
- int oldFreeSpace = io.getFreeSpace(pageAddr);
-
- assert oldFreeSpace >= 0 : oldFreeSpace;
-
- long nextLink = io.removeRow(pageAddr, itemId, pageSize());
-
- int newFreeSpace = io.getFreeSpace(pageAddr);
-
- if (newFreeSpace > MIN_PAGE_FREE_SPACE) {
- int newBucket = bucket(newFreeSpace, false);
-
- boolean putIsNeeded = oldFreeSpace <= MIN_PAGE_FREE_SPACE;
-
- if (!putIsNeeded) {
- int oldBucket = bucket(oldFreeSpace, false);
-
- if (oldBucket != newBucket) {
- // It is possible that page was concurrently taken for
put, in this case put will handle bucket change.
- pageId = maskPartId ?
PageIdUtils.maskPartitionId(pageId) : pageId;
-
- putIsNeeded = removeDataPage(pageId, pageAddr, io,
oldBucket, statHolder);
- }
- }
-
- if (io.isEmpty(pageAddr)) {
- evictionTracker.forgetPage(pageId);
-
- if (putIsNeeded) {
- reuseBag.addFreePage(recyclePage(pageId, pageAddr));
- }
- } else if (putIsNeeded) {
- put(null, pageId, pageAddr, newBucket, statHolder);
- }
- }
-
- // For common case boxed 0L will be cached inside of Long, so no
garbage will be produced.
- return nextLink;
- }
- }
+ private final IoStatisticsHolder statHolder;
/**
* Constructor.
*
* @param grpId Group ID.
* @param partId Partition ID.
- * @param name Structure name (for debug purpose).
* @param pageMem Page memory.
* @param reuseList Reuse list or {@code null} if this free list will be a
reuse list for itself.
* @param lockLsnr Page lock listener.
- * @param log Logger.
* @param metaPageId Metadata page ID.
* @param initNew {@code True} if new metadata should be initialized.
* @param pageListCacheLimit Page list cache limit.
* @param evictionTracker Page eviction tracker.
* @throws IgniteInternalCheckedException If failed.
*/
- public AbstractFreeList(
+ public FreeListImpl(
int grpId,
int partId,
- String name,
PageMemory pageMem,
@Nullable ReuseList reuseList,
PageLockListener lockLsnr,
- IgniteLogger log,
long metaPageId,
boolean initNew,
@Nullable AtomicLong pageListCacheLimit,
- PageEvictionTracker evictionTracker
+ PageEvictionTracker evictionTracker,
+ IoStatisticsHolder statHolder
) throws IgniteInternalCheckedException {
super(
- name,
+ "FreeList_" + grpId,
Review Comment:
Are you sure that it's a good idea to hard-code the name?
FreeLists for different partitions will have the same name, is this
intentional?
--
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]
