[IGNITE-21999] Merge partition free-lists into one #3615

Phillippko · 2024-04-16T13:08:35Z

https://issues.apache.org/jira/browse/IGNITE-21999

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java

ibessonov · 2024-04-23T12:28:29Z

modules/core/src/main/java/org/apache/ignite/internal/util/CachedIterator.java

+import java.util.Iterator;
+
+/** {@link Iterator} implementation that allows to access the current element multiple times. */
+public class CachedIterator<T>  implements Iterator<T> {


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

returned it back to FreeList file

ibessonov · 2024-04-23T12:32:34Z

modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/Storable.java

-    IoVersions<? extends AbstractDataPageIo<?>> ioVersions();
+    IoVersions<DataPageIo> ioVersions();
+
+    /** Returns a byte buffer that contains binary tuple data. */


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?

removed, moved writeRowData and writeFragmentData to storable implementations

modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/Storable.java

modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeList.java

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java

ibessonov · 2024-04-23T12:38:09Z

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java

    ) throws IgniteInternalCheckedException {
        super(
-                name,
+                "FreeList_" + grpId,


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?

Actually it had the same name for partitions before my changes. Made name a parameter, names will have format VolatileFreeList_grpId_partId and PersistentFreeList_grpId_partId

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java

# Conflicts: # modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/datapage/ReadPageMemoryRowValue.java

modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/Storable.java

ibessonov · 2024-05-02T11:00:40Z

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java

+    }
+
+    /** Returns page eviction tracker. */
+    public PageEvictionTracker evictionTracker() {


Is it still required to be public?

Removed this method -> get eviction tracker for handlers through constructors

ibessonov · 2024-05-02T13:41:52Z

...rc/main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryDataRegion.java

    }

    /**
     * Returns version chain free list.
     *
     * @throws StorageException If the data region did not start.
     */
-    public RowVersionFreeList rowVersionFreeList() {
+    public FreeListImpl freeList() {


I wonder why it returns impl and not the interface.
Same applies for places where we declare fields. Your reply would be enough

Sometimes we use this freelist() as ReuseList, not FreeList. It can be changed, but decided to leave it for now

ibessonov · 2024-05-02T13:42:03Z

...rc/main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryDataRegion.java

-    public IndexColumnsFreeList indexColumnsFreeList() {
-        return indexColumnsFreeList;
+    /** Returns pages eviction tracker. */
+    public PageEvictionTracker evictionTracker() {


Is this necessary?

ibessonov · 2024-05-02T13:44:40Z

...-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/MvPageTypes.java

@@ -30,9 +30,6 @@ public interface MvPageTypes {
    /** Version chain tree leaf page IO type. */
    short T_VERSION_CHAIN_LEAF_IO = 11;

-    /** Row version data page IO type. */
-    short T_ROW_VERSION_DATA_IO = 12;


What if we re-assign these numbers, to make them more meaningful? This gap makes no sense now

ibessonov · 2024-05-02T13:45:40Z

...src/main/java/org/apache/ignite/internal/storage/pagememory/index/freelist/IndexColumns.java

 import org.jetbrains.annotations.Nullable;

 /**
 * Index columns to store in free list.
 */
 public class IndexColumns implements Storable {
+    public static final byte DATA_TYPE = 0;


I would prefer it the other way, data being 0 and index being 1

Phillippko changed the title ~~IGNITE-21999 Merge partition free-lists into one~~ [IGNITE-21999] Merge partition free-lists into one Apr 16, 2024

IGNITE-21999 Merge partition free-lists into one

8c24873

Phillippko force-pushed the ignite-21999 branch from 65c7857 to 8c24873 Compare April 16, 2024 13:43

IGNITE-21999 fixes

4acb234

Phillippko marked this pull request as ready for review April 16, 2024 14:16

ibessonov reviewed Apr 16, 2024

View reviewed changes

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java Show resolved Hide resolved

...s/page-memory/src/main/java/org/apache/ignite/internal/pagememory/freelist/FreeListImpl.java Outdated Show resolved Hide resolved

Phillippko added 11 commits April 17, 2024 18:03

IGNITE-21999 merge DataPageIo implementations

b233247

IGNITE-21999 fixes

d3fded8

IGNITE-21999 add data type

f37c164

IGNITE-21999 fixes

c52fc74

IGNITE-21999 fixes

3509691

IGNITE-21999 fixes

1f1a487

IGNITE-21999 make own class for WriteRowHandler

c1f2513

IGNITE-21999 move inner classes to outer level

0f04c4a

IGNITE-21999 fix mpd

7056d48

Merge branch 'main' into ignite-21999

2411fc4

IGNITE-21999 javadocs

43816f7

ibessonov requested changes Apr 23, 2024

View reviewed changes

Phillippko added 9 commits May 1, 2024 15:51

ignite-21999 review fixes

141f0f1

Merge remote-tracking branch 'origin/ignite-21999' into ignite-21999

fe29873

# Conflicts: # modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/datapage/ReadPageMemoryRowValue.java

ignite-21999 remove default methods

c953720

ignite-21999 revert changes

ee24f02

ignite-21999 revert changes

2c27776

ignite-21999 revert changes

bea0c40

ignite-21999 fix

86f61cb

ignite-21999 fix

f46f07b

ignite-21999 fix

614b73a

ibessonov reviewed May 2, 2024

View reviewed changes

Phillippko added 2 commits May 2, 2024 22:10

IGNITE-21999 review fixes

5a3d049

IGNITE-21999 fix

a8b9a10

IGNITE-21999 fix

817c898

ibessonov approved these changes May 3, 2024

View reviewed changes

rpuch merged commit 7f93d63 into apache:main May 6, 2024
1 check passed

rpuch deleted the ignite-21999 branch May 6, 2024 09:29

AMashenkov pushed a commit that referenced this pull request May 14, 2024

[IGNITE-21999] Merge partition free-lists into one (#3615)

404afe3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IGNITE-21999] Merge partition free-lists into one #3615

[IGNITE-21999] Merge partition free-lists into one #3615

Phillippko commented Apr 16, 2024

ibessonov Apr 23, 2024

Phillippko May 2, 2024

ibessonov Apr 23, 2024

Phillippko May 2, 2024

ibessonov Apr 23, 2024

Phillippko May 2, 2024

ibessonov May 2, 2024

Phillippko May 2, 2024

ibessonov May 2, 2024

Phillippko May 2, 2024

ibessonov May 2, 2024

ibessonov May 2, 2024

ibessonov May 2, 2024

[IGNITE-21999] Merge partition free-lists into one #3615

[IGNITE-21999] Merge partition free-lists into one #3615

Conversation

Phillippko commented Apr 16, 2024

The Review Checklist

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment