IGNITE-17132 [Native Persistence 3.0] Implement partition destruction for persistent PageMemory #1325

tkalkirill · 2022-11-08T17:40:43Z

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

rpuch · 2022-11-22T07:03:05Z

.../main/java/org/apache/ignite/internal/pagememory/persistence/PartitionProcessingCounter.java

+
+    static {
+        try {
+            COUNTER = MethodHandles.lookup().findVarHandle(PartitionProcessingCounter.class, "counter", int.class);


It looks like a replacement for an AtomicInteger. AtomicInteger would make the code more concise and easier to understand, you could use incrementAndGet() and decrementAndGet() below.

You probably wanted to spare object instances using this approach, but it's just one instance per partition. It seems unlikely that someone will have even a million partitions in the same JVM, so the saving is not worth code complexity.

In this case, we will save memory and at the same time the complexity of the code will not increase.

The complexity increases in both creation (static block with try and MethodHandles invocation vs new AtomicInteger()) and usage (incrementAndGet() vs getAndAdd(this, 1) + 1). It is slightly simpler, but simpler. And even slight simplification is wortha lot (surely more than saving 10k objects per JVM).

I do not think that for us it greatly complicates the code.

rpuch · 2022-11-22T07:03:42Z

.../main/java/org/apache/ignite/internal/pagememory/persistence/PartitionProcessingCounter.java

+    public void onStartPartitionProcessing() {
+        assert !future.isDone();
+
+        int updatedValue = (int) COUNTER.getAndAdd(this, 1) + 1;


Looks analogous to counter.incrementAndGet() (when using AtomicInteger)

It is, but it's not complex code.

rpuch · 2022-11-22T07:03:52Z

.../main/java/org/apache/ignite/internal/pagememory/persistence/PartitionProcessingCounter.java

+    public void onFinishPartitionProcessing() {
+        assert !future.isDone();
+
+        int updatedValue = (int) COUNTER.getAndAdd(this, -1) - 1;


Looks analogous to counter.decrementAndGet() (when using AtomicInteger)

It is, but it's not complex code.

.../main/java/org/apache/ignite/internal/pagememory/persistence/PartitionProcessingCounter.java

...in/java/org/apache/ignite/internal/pagememory/persistence/PartitionProcessingCounterMap.java

...org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java

rpuch · 2022-11-22T14:36:59Z

...ava/org/apache/ignite/internal/storage/pagememory/index/hash/PageMemoryHashIndexStorage.java

+
+    static {
+        try {
+            STARTED = MethodHandles.lookup().findVarHandle(PageMemoryHashIndexStorage.class, "started", boolean.class);


Let's use AtomicBoolean instead. It makes the code simpler and the price we pay (one object per index per partition) is tiny. The same relates to all pagemem-based index and MvPartitionStorage implementations.

I think we should leave it as it is, we use less memory, and the complexity here is minimal.

rpuch · 2022-11-22T14:38:35Z

...va/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryMvTableStorageTest.java

 */
-@ExtendWith(WorkDirectoryExtension.class)
-@ExtendWith(ConfigurationExtension.class)
+@ExtendWith({ConfigurationExtension.class, WorkDirectoryExtension.class})


Both styles seem ok, but why is this (one annotation with an array) seems better to you? It brings some syntactic noise (braces).

It's just that I like it better, I suggest leaving it as it is.

...va/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryMvTableStorageTest.java

rpuch · 2022-11-22T14:53:16Z

Also, I wonder, why LongOperationAsyncExecutor launches a fresh thread per async operation instead of using a single-thread executor to run both main tasks and afterAsyncCompletion tasks. This is not in the scope of this PR, I'm just curious.

.../main/java/org/apache/ignite/internal/pagememory/persistence/PartitionProcessingCounter.java

...ain/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointManager.java

...g/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointPagesWriterFactory.java

...est/java/org/apache/ignite/internal/pagememory/persistence/store/GroupPageStoresMapTest.java

ibessonov · 2022-11-22T12:23:13Z

...les/storage-api/src/main/java/org/apache/ignite/internal/storage/StorageClosedException.java

+     * @param cause The cause.
+     */
+    public StorageClosedException(String message, Throwable cause) {
+        super(message, cause);


Where's the error code? :)
I guess we should make it soon

coming soon

ibessonov · 2022-11-22T12:24:31Z

modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java

+     * <p>This method will do nothing if there is no partition by ID, when trying to call methods to read or write (as well as all previous
+     * open cursors) for {@link MvPartitionStorage}, {@link HashIndexStorage} and {@link SortedIndexStorage}, {@link StorageClosedException}
+     * will be thrown.
+     *


This description is close to impossible to understand, please use re-structure your text and expand it.

What exactly is not clear?

The grammatical structure is very convoluted. Maybe something like

This method will ... if: - blah - foo - bar

will be easier to read

Deleted this comment.

ibessonov · 2022-11-22T12:26:41Z

...api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java

+        assertThrows(StorageClosedException.class, storage::lastAppliedIndex);
+        assertThrows(StorageClosedException.class, storage::lastAppliedTerm);


These two don't have to throw exceptions, but ok

...ain/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryTableStorage.java

.../main/java/org/apache/ignite/internal/pagememory/persistence/store/FilePageStoreManager.java

...src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/Checkpointer.java

...ry/src/main/java/org/apache/ignite/internal/pagememory/persistence/compaction/Compactor.java

rpuch · 2022-11-23T10:15:21Z

...ry/src/main/java/org/apache/ignite/internal/pagememory/persistence/compaction/Compactor.java

            return;
        }

+        if (filePageStore.isMarkedToDestroy()) {


These checks (whether it's cancelled, whether the store is marked for destroy) are always done together in this method. How about uniting them in a single method like shouldNotProceedWith(FilePageStore) and call it in all places in this method?

But for what?

To make sure that we always do these checks together in this method. After modifications someone might accidentally add just one check instead of 2.

I think we do not need a separate method for two slightly different checks, this complicates the code a bit.

ibessonov · 2022-11-23T10:17:21Z

...ain/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryTableStorage.java

+
+        CompletableFuture<Void> previousFuture = destroyFutureByPartitionId.put(partitionId, new CompletableFuture<>());
+
+        assert previousFuture == null : "Previous destruction of the partition has not completed: " + partitionId;


Why not just return the old future?

What's up with the future anyway? No one uses it, what's the point?

I don’t understand why return the current future while no one uses it, but later they will be on the assignment recalculation.

I see, didn't notice the other usage

.../main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryTableStorage.java

...ava/org/apache/ignite/internal/storage/pagememory/index/hash/PageMemoryHashIndexStorage.java

...org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java

...a/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java

...src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/Checkpointer.java

...ry/src/main/java/org/apache/ignite/internal/pagememory/persistence/compaction/Compactor.java

...api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java

.../main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java

.../apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java

rpuch

.

...src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/Checkpointer.java

.../main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java

...ge-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java

… for persistent PageMemory (apache#1325)

tkalkirill added 30 commits November 8, 2022 20:34

IGNITE-17132 wip

047ccf4

IGNITE-17132 wip

0402f50

IGNITE-17132 wip

9aaf913

IGNITE-17132 wip

06c972a

IGNITE-17132 wip

2d53ce6

IGNITE-17132 wip

0f91935

IGNITE-17132 wip

8ef0e7c

Merge branch 'ai-main' into ignite-17132

e0bed0c

IGNITE-17132 wip

4dc7bcc

Merge branch 'ai-main' into ignite-17132

e3ede48

Merge branch 'ai-main' into ignite-17132

89277a1

IGNITE-17132 after merge ai-main

b6acce0

Merge branch 'ai-main' into ignite-17132

c79b769

IGNITE-17132 revert some changes

2e217a2

IGNITE-17132 revert some changes #2.0

37d77f9

IGNITE-17132 wip

c3fa0a3

IGNITE-17132 wip

26868ce

IGNITE-17132 wip

9899fd5

IGNITE-17132 wip

cf15513

IGNITE-17132 wip

77281a9

Merge branch 'ai-main' into ignite-17132

8b13c0d

IGNITE-17132 wip

f5818ed

Merge branch 'ai-main' into ignite-17132

27d16eb

IGNITE-17132 wip

c7143ff

IGNITE-17132 wip

2a0cf92

IGNITE-17132 wip

bde3bf9

IGNITE-17132 wip

252391a

IGNITE-17132 wip

5127312

IGNITE-17132 wip

2689e36

Merge branch 'ai-main' into ignite-17132

2e3cfc4

tkalkirill added 3 commits November 21, 2022 15:50

IGNITE-17132 after merge ai-main

af636d0

IGNITE-17132 wip

e2e781c

Merge branch 'ai-main' into ignite-17132

04c31f3

rpuch reviewed Nov 22, 2022

View reviewed changes

ibessonov reviewed Nov 23, 2022

View reviewed changes

rpuch reviewed Nov 23, 2022

View reviewed changes

.../main/java/org/apache/ignite/internal/pagememory/persistence/store/FilePageStoreManager.java Outdated Show resolved Hide resolved

...src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/Checkpointer.java Outdated Show resolved Hide resolved

rpuch reviewed Nov 23, 2022

View reviewed changes

ibessonov reviewed Nov 23, 2022

View reviewed changes

tkalkirill added 6 commits November 24, 2022 14:24

IGNITE-17132 after review #1.0

1cc986d

Merge branch 'ai-main' into ignite-17132

5936dbe

IGNITE-17132 after review #1.1

9255b85

Merge branch 'ai-main' into ignite-17132

2380e96

IGNITE-17132 after review #1.2

152ccd5

Merge branch 'ai-main' into ignite-17132

b1177b7

rpuch reviewed Nov 25, 2022

View reviewed changes

IGNITE-17132 after review #2.0

bbd0730

rpuch reviewed Nov 25, 2022

View reviewed changes

rpuch reviewed Nov 28, 2022

View reviewed changes

tkalkirill added 4 commits November 28, 2022 12:32

IGNITE-17132 after review #3.0

71816e2

Merge branch 'ai-main' into ignite-17132

d72cca1

Merge branch 'ai-main' into ignite-17132

4136ac1

IGNITE-17132 after pmd checks

bd7b897

rpuch approved these changes Nov 28, 2022

View reviewed changes

tkalkirill merged commit 0dee583 into apache:main Nov 28, 2022

tkalkirill deleted the ignite-17132 branch November 28, 2022 10:42

lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023

IGNITE-17132 [Native Persistence 3.0] Implement partition destruction…

ea0d455

… for persistent PageMemory (apache#1325)

lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023

IGNITE-17132 [Native Persistence 3.0] Implement partition destruction…

195f24a

… for persistent PageMemory (apache#1325)

		assertThrows(StorageClosedException.class, storage::lastAppliedIndex);
		assertThrows(StorageClosedException.class, storage::lastAppliedTerm);


		CompletableFuture<Void> previousFuture = destroyFutureByPartitionId.put(partitionId, new CompletableFuture<>());

		assert previousFuture == null : "Previous destruction of the partition has not completed: " + partitionId;

IGNITE-17132 [Native Persistence 3.0] Implement partition destruction for persistent PageMemory #1325

IGNITE-17132 [Native Persistence 3.0] Implement partition destruction for persistent PageMemory #1325

Uh oh!

Conversation

tkalkirill commented Nov 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpuch commented Nov 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkalkirill Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tkalkirill Nov 28, 2022 •

edited

Loading