[core] Support commit isolation level #3805

BsoBird · 2024-07-24T02:46:27Z

Purpose

For the file system catalog, there are many situations where we need to support stricter commit policies.This is especially true for third-party databases that use paimon as their underlying storage.

We define the commit isolation level option to control the behavior.

Linked issue: none

Disable retry after failed commits in strict mode
In strict mode, reject and clean up potentially dirty commits.
Categorized possible exceptions and adjusted the exception handling logic.
Supports concurrency control.
In strict mode, do not use any hint file.

Tests

See FileStoreCommitTest.java.

API and Format

None

Documentation

Glossary:

Dirty-commit: commitSnapshotId < CurrentLatestSnapshotId - snapshot.num-retained.max && commit-success=True && snapshotManager.exists(commitSnapshotId)=True && (commitSnapshotId not belong any tag/branch)

Flow chat (use strict mode):

    graph TD
START[startCommit] -->USE-LOCK{USE-LOCK?}
USE-LOCK -->|YES| COMMIT_WITH_LOCK{COMMIT_WITH_LOCK_SUCCESS}
USE-LOCK -->|NO| CHECK-BEFORE{CHECK-BEFORE-SUCCESS?}
CHECK-BEFORE -->|NO| COMMIT_FAILED
CHECK-BEFORE -->|YES| COMMIT{COMMIT_SUCCESS?}
COMMIT --> |commit state unknown| COMMIT_FAILED
COMMIT --> |not success| COMMIT_FAILED
COMMIT --> |YES| IS_DIRTY_COMMIT{IS_DIRTY_COMMIT?}
IS_DIRTY_COMMIT --> |YES| COMMIT_FAILED
IS_DIRTY_COMMIT --> |NO| COMMIT_SUCCESS
COMMIT_WITH_LOCK --> |failed-normal| RETRY
COMMIT_WITH_LOCK --> |failed - strict mode| COMMIT_FAILED
COMMIT_WITH_LOCK --> |success| COMMIT_SUCCESS
RETRY --> END
COMMIT_SUCCESS --> END
COMMIT_FAILED --> END

BsoBird · 2024-07-24T02:47:51Z

@JingsongLi Hi. Can you check this? Tks.

BsoBird · 2024-07-24T03:25:59Z

paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java

@@ -238,6 +238,9 @@ default boolean tryToWriteAtomic(Path path, String content) throws IOException {
        try {
            writeFile(tmp, content, false);
            success = rename(tmp, path);
+        } catch (IOException e) {
+            // try check once
+            success = exists(path) && !exists(tmp);


After throwing an IO exception, it's a good idea to check it again.Currently, this checking strategy is not fully applicable to object storage systems and may result in false positives.However, as the industry evolves, object storage systems are starting to support mutex operations.In the long run, I think we need to add check operations.

However, please note that we can only check for IO exceptions after they are caught, because they are thrown by the server, and we should not do any checking for client-side exceptions.

This can be a separate PR?

This can be a separate PR?

sure

BsoBird · 2024-07-24T03:27:26Z

paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java

+            // otherwise, we throw an exception.
+            if (!success && useSerializableIsolation) {
+                throw new CommitFailedException(e.getMessage(), e);
+            }


If the rename operation succeeds, we should ignore any exceptions thrown after that.

BsoBird · 2024-07-24T03:28:54Z

paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java

+                if (useSerializableIsolation && !useLock) {
+                    // If the useSerializableIsolation is turned on,And we not use lock service,
+                    // we will remove all HINTs,as it may provide incorrect information.
+                    snapshotManager.removeSnapshotEarliestHint();


If Serializable is turned on, and the locking service is not used, we should delete all HINT files, as they are always unreliable.

BsoBird · 2024-07-24T03:29:31Z

paimon-core/src/main/java/org/apache/paimon/utils/BranchManager.java

@@ -284,7 +284,7 @@ public void fastForward(String branchName) {
                            .collect(Collectors.toList());

            // Delete latest snapshot hint
-            snapshotManager.deleteLatestHint();
+            snapshotManager.removeSnapshotLatestHint();


The previous method name wasn't clear enough, so I've replaced it

BsoBird · 2024-07-24T03:30:22Z

paimon-core/src/test/java/org/apache/paimon/TestFileStore.java

-            snapshots.add(snapshotManager.snapshot(id));
+            if (snapshotManager.snapshotExists(id)) {
+                snapshots.add(snapshotManager.snapshot(id));
+            }


If a dirty commit occurs, the previous processing logic throws an exception, which affects the running of the test case.

BsoBird · 2024-07-24T03:34:05Z

paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java

+            }
+            return committed;
+        } catch (Exception e) {
+            throw new CommitStateUnknownException(e.getMessage(), e);


If any exception occurs during the commit process, we should throw a CommitStateUnknownException and not clean up any data.

BsoBird · 2024-07-24T05:48:31Z

paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java

+            throw e;
+        } catch (DirtyCommitException e) {
+            // We need to clean up all the metadata information generated by this commit.
+            cleanUpTmpManifests(


Maybe cleaning up dirty commits will interfere with the creation of branch/tag. But since we are always cleaning up snapshots that exceed snapshot.num-retained.max.This shouldn't have much of an impact. After all, if we keep a snapshot that matches the characteristics of a dirty commit, we may never find the right time to delete it.

BsoBird · 2024-07-24T06:03:14Z

paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java

+                latestSnapshotIdAfterCommit > maxSaveSnapshotNum
+                        ? latestSnapshotIdAfterCommit - maxSaveSnapshotNum
+                        : -1;
+        fastFailIfDirtyCommit(newSnapshotId, waterMark, latestSnapshotIdAfterCommit);


Here we need to add the logic of whether the snapshot belongs to a branch or tag.PR 3787 (#3787) provides this functionality, I'm not sure if I need to merge it with that PR?

For now I'm starting by porting some of the code from 3787 to the current PR.

JingsongLi · 2024-07-30T13:54:03Z

Thanks @BsoBird ! I need time to look this, next week will give your feedback.

JingsongLi

Hi @BsoBird , I have no idea why the changes is so big...

In my imagination, we only need one branch to complete the new commit mode.
Please note that this is the core code, and any modifications may result in serious consequences.

BsoBird added 2 commits July 24, 2024 10:09

Support multiple isolation level

8e72137

Merge branch 'apache:master' into supportCommitIsolationLevel

2fcb97a

BsoBird changed the title ~~Support commit isolation level~~ [core] Support commit isolation level Jul 24, 2024

BsoBird commented Jul 24, 2024

View reviewed changes

Support multiple isolation level

f157d6c

BsoBird added 2 commits July 31, 2024 17:09

Support multiple isolation level

d1b7edb

change clean logic

b6d3625

JingsongLi requested changes Aug 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Support commit isolation level #3805

[core] Support commit isolation level #3805

BsoBird commented Jul 24, 2024 •

edited

Loading

BsoBird commented Jul 24, 2024

BsoBird Jul 24, 2024 •

edited

Loading

JingsongLi Aug 12, 2024

BsoBird Aug 12, 2024

BsoBird Jul 24, 2024

BsoBird Jul 24, 2024

BsoBird Jul 24, 2024 •

edited

Loading

BsoBird Jul 24, 2024

BsoBird Jul 24, 2024

BsoBird Jul 24, 2024

BsoBird Jul 24, 2024 •

edited

Loading

BsoBird Jul 24, 2024

JingsongLi commented Jul 30, 2024

JingsongLi left a comment

[core] Support commit isolation level #3805

Are you sure you want to change the base?

[core] Support commit isolation level #3805

Conversation

BsoBird commented Jul 24, 2024 • edited Loading

Purpose

Tests

API and Format

Documentation

Glossary:

Flow chat (use strict mode):

BsoBird commented Jul 24, 2024

BsoBird Jul 24, 2024 • edited Loading

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

BsoBird Jul 24, 2024 • edited Loading

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

BsoBird Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JingsongLi commented Jul 30, 2024

JingsongLi left a comment

Choose a reason for hiding this comment

BsoBird commented Jul 24, 2024 •

edited

Loading

BsoBird Jul 24, 2024 •

edited

Loading

BsoBird Jul 24, 2024 •

edited

Loading

BsoBird Jul 24, 2024 •

edited

Loading