You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "BsoBird (via GitHub)" <gi...@apache.org> on 2024/07/24 02:46:30 UTC
[PR] Support commit isolation level [paimon]
BsoBird opened a new pull request, #3805:
URL: https://github.com/apache/paimon/pull/3805
<!-- Please specify the module before the PR name: [core] ... or [flink] ... -->
### 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.
<!-- Linking this pull request to the issue -->
Linked issue: none
<!-- What is the purpose of the change -->
1. Disable retry after failed commits in strict mode
2. In strict mode, reject and clean up potentially dirty commits.
3. Categorized possible exceptions and adjusted the exception handling logic.
4. Supports concurrency control.
### Tests
<!-- List UT and IT cases to verify this change -->
See FileStoreCommitTest.java.
### API and Format
<!-- Does this change affect API or storage format -->
None
### Documentation
<!-- Does this change introduce a new feature -->
#### Glossary:
- Dirty-commit: commitSnapshotId < CurrentLatestSnapshotId - snapshot.num-retained.max && commit-success=True && snapshotManager.exists(commitSnapshotId)=True
#### Flow chat (use strict mode):
```mermaid
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 --> |normal| RETRY
COMMIT_WITH_LOCK --> |strict mode| COMMIT_FAILED
RETRY --> END
COMMIT_SUCCESS --> END
COMMIT_FAILED --> END
```
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi closed pull request #3805: [core] Support commit isolation level
URL: https://github.com/apache/paimon/pull/3805
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689265509
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -1274,4 +1308,101 @@ static ConflictCheck noConflictCheck() {
public static ConflictCheck mustConflictCheck() {
return latestSnapshot -> true;
}
+
+ boolean doCommit(
+ long newSnapshotId,
+ Path newSnapshotPath,
+ Snapshot newSnapshot,
+ boolean useSerializableIsolation,
+ boolean useLock) {
+ try {
+ boolean committed = tryAtomicCommitNewSnapshot(newSnapshotPath, newSnapshot);
+ if (committed) {
+ 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();
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeChangeLogEarliestHint();
+ snapshotManager.removeChangeLogLatestHint();
+ } else {
+ snapshotManager.commitLatestHint(newSnapshotId);
+ }
+ }
+ return committed;
+ } catch (Exception e) {
+ throw new CommitStateUnknownException(e.getMessage(), e);
+ }
+ }
+
+ boolean tryAtomicCommitNewSnapshot(Path newSnapshotPath, Snapshot newSnapshot)
+ throws IOException {
+ return fileIO.tryToWriteAtomic(newSnapshotPath, newSnapshot.toJson());
+ }
+
+ void checkBeforeCommit(long newSnapshotId) throws IOException {
+ long latestSnapshotIdBeforeCommit =
+ Optional.ofNullable(snapshotManager.latestSnapshotIdWithOutHint())
+ .orElse(Snapshot.FIRST_SNAPSHOT_ID - 1);
+ boolean currentCommitIsLatest =
+ Objects.equals(newSnapshotId, latestSnapshotIdBeforeCommit + 1);
+ if (!currentCommitIsLatest) {
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeSnapshotEarliestHint();
+ throw new CommitFailedException(
+ String.format(
+ "Can't submit snapshotId [%s], because it is expected that snapshotId [%s] should be submitted now.",
+ newSnapshotId, latestSnapshotIdBeforeCommit + 1));
+ }
+ }
+
+ void checkAfterCommit(long newSnapshotId) throws IOException {
+ String maxSaveSnapshotsNumStr = tableOptions.get(SNAPSHOT_NUM_RETAINED_MAX.key());
+ int maxSaveSnapshotNum = SNAPSHOT_NUM_RETAINED_MAX.defaultValue();
+ if (!StringUtils.isBlank(maxSaveSnapshotsNumStr)) {
+ maxSaveSnapshotNum = Integer.parseInt(maxSaveSnapshotsNumStr);
+ }
+ long latestSnapshotIdAfterCommit =
+ Optional.ofNullable(snapshotManager.latestSnapshotIdWithOutHint())
+ .orElse(Snapshot.FIRST_SNAPSHOT_ID);
+ long waterMark =
+ latestSnapshotIdAfterCommit > maxSaveSnapshotNum
+ ? latestSnapshotIdAfterCommit - maxSaveSnapshotNum
+ : -1;
+ fastFailIfDirtyCommit(newSnapshotId, waterMark, latestSnapshotIdAfterCommit);
Review Comment:
For now I'm starting by porting some of the code from 3787 to the current PR.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689184744
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -1274,4 +1308,101 @@ static ConflictCheck noConflictCheck() {
public static ConflictCheck mustConflictCheck() {
return latestSnapshot -> true;
}
+
+ boolean doCommit(
+ long newSnapshotId,
+ Path newSnapshotPath,
+ Snapshot newSnapshot,
+ boolean useSerializableIsolation,
+ boolean useLock) {
+ try {
+ boolean committed = tryAtomicCommitNewSnapshot(newSnapshotPath, newSnapshot);
+ if (committed) {
+ 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();
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeChangeLogEarliestHint();
+ snapshotManager.removeChangeLogLatestHint();
+ } else {
+ snapshotManager.commitLatestHint(newSnapshotId);
+ }
+ }
+ return committed;
+ } catch (Exception e) {
+ throw new CommitStateUnknownException(e.getMessage(), e);
+ }
+ }
+
+ boolean tryAtomicCommitNewSnapshot(Path newSnapshotPath, Snapshot newSnapshot)
+ throws IOException {
+ return fileIO.tryToWriteAtomic(newSnapshotPath, newSnapshot.toJson());
+ }
+
+ void checkBeforeCommit(long newSnapshotId) throws IOException {
+ long latestSnapshotIdBeforeCommit =
+ Optional.ofNullable(snapshotManager.latestSnapshotIdWithOutHint())
+ .orElse(Snapshot.FIRST_SNAPSHOT_ID - 1);
+ boolean currentCommitIsLatest =
+ Objects.equals(newSnapshotId, latestSnapshotIdBeforeCommit + 1);
+ if (!currentCommitIsLatest) {
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeSnapshotEarliestHint();
+ throw new CommitFailedException(
+ String.format(
+ "Can't submit snapshotId [%s], because it is expected that snapshotId [%s] should be submitted now.",
+ newSnapshotId, latestSnapshotIdBeforeCommit + 1));
+ }
+ }
+
+ void checkAfterCommit(long newSnapshotId) throws IOException {
+ String maxSaveSnapshotsNumStr = tableOptions.get(SNAPSHOT_NUM_RETAINED_MAX.key());
+ int maxSaveSnapshotNum = SNAPSHOT_NUM_RETAINED_MAX.defaultValue();
+ if (!StringUtils.isBlank(maxSaveSnapshotsNumStr)) {
+ maxSaveSnapshotNum = Integer.parseInt(maxSaveSnapshotsNumStr);
+ }
+ long latestSnapshotIdAfterCommit =
+ Optional.ofNullable(snapshotManager.latestSnapshotIdWithOutHint())
+ .orElse(Snapshot.FIRST_SNAPSHOT_ID);
+ long waterMark =
+ latestSnapshotIdAfterCommit > maxSaveSnapshotNum
+ ? latestSnapshotIdAfterCommit - maxSaveSnapshotNum
+ : -1;
+ fastFailIfDirtyCommit(newSnapshotId, waterMark, latestSnapshotIdAfterCommit);
Review Comment:
Here we need to add the logic of whether the snapshot belongs to a branch or tag.PR 3787 provides this functionality, I'm not sure if I need to merge it with that PR?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689184744
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -1274,4 +1308,101 @@ static ConflictCheck noConflictCheck() {
public static ConflictCheck mustConflictCheck() {
return latestSnapshot -> true;
}
+
+ boolean doCommit(
+ long newSnapshotId,
+ Path newSnapshotPath,
+ Snapshot newSnapshot,
+ boolean useSerializableIsolation,
+ boolean useLock) {
+ try {
+ boolean committed = tryAtomicCommitNewSnapshot(newSnapshotPath, newSnapshot);
+ if (committed) {
+ 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();
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeChangeLogEarliestHint();
+ snapshotManager.removeChangeLogLatestHint();
+ } else {
+ snapshotManager.commitLatestHint(newSnapshotId);
+ }
+ }
+ return committed;
+ } catch (Exception e) {
+ throw new CommitStateUnknownException(e.getMessage(), e);
+ }
+ }
+
+ boolean tryAtomicCommitNewSnapshot(Path newSnapshotPath, Snapshot newSnapshot)
+ throws IOException {
+ return fileIO.tryToWriteAtomic(newSnapshotPath, newSnapshot.toJson());
+ }
+
+ void checkBeforeCommit(long newSnapshotId) throws IOException {
+ long latestSnapshotIdBeforeCommit =
+ Optional.ofNullable(snapshotManager.latestSnapshotIdWithOutHint())
+ .orElse(Snapshot.FIRST_SNAPSHOT_ID - 1);
+ boolean currentCommitIsLatest =
+ Objects.equals(newSnapshotId, latestSnapshotIdBeforeCommit + 1);
+ if (!currentCommitIsLatest) {
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeSnapshotEarliestHint();
+ throw new CommitFailedException(
+ String.format(
+ "Can't submit snapshotId [%s], because it is expected that snapshotId [%s] should be submitted now.",
+ newSnapshotId, latestSnapshotIdBeforeCommit + 1));
+ }
+ }
+
+ void checkAfterCommit(long newSnapshotId) throws IOException {
+ String maxSaveSnapshotsNumStr = tableOptions.get(SNAPSHOT_NUM_RETAINED_MAX.key());
+ int maxSaveSnapshotNum = SNAPSHOT_NUM_RETAINED_MAX.defaultValue();
+ if (!StringUtils.isBlank(maxSaveSnapshotsNumStr)) {
+ maxSaveSnapshotNum = Integer.parseInt(maxSaveSnapshotsNumStr);
+ }
+ long latestSnapshotIdAfterCommit =
+ Optional.ofNullable(snapshotManager.latestSnapshotIdWithOutHint())
+ .orElse(Snapshot.FIRST_SNAPSHOT_ID);
+ long waterMark =
+ latestSnapshotIdAfterCommit > maxSaveSnapshotNum
+ ? latestSnapshotIdAfterCommit - maxSaveSnapshotNum
+ : -1;
+ fastFailIfDirtyCommit(newSnapshotId, waterMark, latestSnapshotIdAfterCommit);
Review Comment:
Here we need to add the logic of whether the snapshot belongs to a branch or tag.PR 3787 (https://github.com/apache/paimon/pull/3787) provides this functionality, I'm not sure if I need to merge it with that PR?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689057953
##########
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();
Review Comment:
The previous method wasn't clear enough, so I've replaced it
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1713731762
##########
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);
Review Comment:
> This can be a separate PR?
sure
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689057016
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -956,46 +986,46 @@ public boolean tryCommitOnce(
// case
!fileIO.exists(newSnapshotPath) && callable.call());
} else {
+ if (useSerializableIsolation) {
+ checkBeforeCommit(newSnapshotId);
+ }
success = callable.call();
+ if (useSerializableIsolation && success) {
+ checkAfterCommit(newSnapshotId);
+ }
}
- } catch (Throwable e) {
+ } catch (CommitStateUnknownException e) {
// exception when performing the atomic rename,
// we cannot clean up because we can't determine the success
- throw new RuntimeException(
- String.format(
- "Exception occurs when committing snapshot #%d (path %s) by user %s "
- + "with identifier %s and kind %s. "
- + "Cannot clean up because we can't determine the success.",
- newSnapshotId,
- newSnapshotPath,
- commitUser,
- identifier,
- commitKind.name()),
- e);
+ throw e;
+ } catch (DirtyCommitException e) {
+ // We need to clean up all the metadata information generated by this commit.
+ cleanUpTmpManifests(
+ previousChangesListName,
+ newChangesListName,
+ changelogListName,
+ newIndexManifest,
+ oldMetas,
+ newMetas,
+ changelogMetas);
+ throw e;
+ } catch (Exception e) {
+ // If the commit succeeds, we ignore all exceptions thrown by the successful commit;
+ // otherwise, we throw an exception.
+ if (!success && useSerializableIsolation) {
+ throw new CommitFailedException(e.getMessage(), e);
+ }
Review Comment:
If the rename operation succeeds, we should ignore any exceptions thrown after that.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689058409
##########
paimon-core/src/test/java/org/apache/paimon/TestFileStore.java:
##########
@@ -363,7 +363,9 @@ public List<Snapshot> commitDataImpl(
List<Snapshot> snapshots = new ArrayList<>();
for (long id = snapshotIdBeforeCommit + 1; id <= snapshotIdAfterCommit; id++) {
- snapshots.add(snapshotManager.snapshot(id));
+ if (snapshotManager.snapshotExists(id)) {
+ snapshots.add(snapshotManager.snapshot(id));
+ }
Review Comment:
If a dirty commit occurs, the previous processing logic throws an exception, which affects the running of the test case.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689056204
##########
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);
Review Comment:
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 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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on PR #3805:
URL: https://github.com/apache/paimon/pull/3805#issuecomment-2246758812
@JingsongLi Hi. Can you check this? Tks.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689057953
##########
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();
Review Comment:
The previous method name wasn't clear enough, so I've replaced it
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #3805:
URL: https://github.com/apache/paimon/pull/3805#issuecomment-2705551119
Close this PR because long it is no longer active. Feel free to open a discussion in dev mail list.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1713342931
##########
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);
Review Comment:
This can be a separate PR?
##########
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);
Review Comment:
This can be a separate PR?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1713342931
##########
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);
Review Comment:
This can be a separate PR?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689056204
##########
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);
Review Comment:
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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689172657
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -956,46 +986,46 @@ public boolean tryCommitOnce(
// case
!fileIO.exists(newSnapshotPath) && callable.call());
} else {
+ if (useSerializableIsolation) {
+ checkBeforeCommit(newSnapshotId);
+ }
success = callable.call();
+ if (useSerializableIsolation && success) {
+ checkAfterCommit(newSnapshotId);
+ }
}
- } catch (Throwable e) {
+ } catch (CommitStateUnknownException e) {
// exception when performing the atomic rename,
// we cannot clean up because we can't determine the success
- throw new RuntimeException(
- String.format(
- "Exception occurs when committing snapshot #%d (path %s) by user %s "
- + "with identifier %s and kind %s. "
- + "Cannot clean up because we can't determine the success.",
- newSnapshotId,
- newSnapshotPath,
- commitUser,
- identifier,
- commitKind.name()),
- e);
+ throw e;
+ } catch (DirtyCommitException e) {
+ // We need to clean up all the metadata information generated by this commit.
+ cleanUpTmpManifests(
Review Comment:
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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689060183
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -1274,4 +1308,101 @@ static ConflictCheck noConflictCheck() {
public static ConflictCheck mustConflictCheck() {
return latestSnapshot -> true;
}
+
+ boolean doCommit(
+ long newSnapshotId,
+ Path newSnapshotPath,
+ Snapshot newSnapshot,
+ boolean useSerializableIsolation,
+ boolean useLock) {
+ try {
+ boolean committed = tryAtomicCommitNewSnapshot(newSnapshotPath, newSnapshot);
+ if (committed) {
+ 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();
+ snapshotManager.removeSnapshotLatestHint();
+ snapshotManager.removeChangeLogEarliestHint();
+ snapshotManager.removeChangeLogLatestHint();
+ } else {
+ snapshotManager.commitLatestHint(newSnapshotId);
+ }
+ }
+ return committed;
+ } catch (Exception e) {
+ throw new CommitStateUnknownException(e.getMessage(), e);
Review Comment:
If any exception occurs during the commit process, we should throw a CommitStateUnknownException and not clean up any data.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689057710
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -1274,4 +1308,101 @@ static ConflictCheck noConflictCheck() {
public static ConflictCheck mustConflictCheck() {
return latestSnapshot -> true;
}
+
+ boolean doCommit(
+ long newSnapshotId,
+ Path newSnapshotPath,
+ Snapshot newSnapshot,
+ boolean useSerializableIsolation,
+ boolean useLock) {
+ try {
+ boolean committed = tryAtomicCommitNewSnapshot(newSnapshotPath, newSnapshot);
+ if (committed) {
+ 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();
Review Comment:
If Serializable is turned on, and the locking service is not used, we should delete all HINT files, as they are always unreliable.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "BsoBird (via GitHub)" <gi...@apache.org>.
BsoBird commented on code in PR #3805:
URL: https://github.com/apache/paimon/pull/3805#discussion_r1689057953
##########
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();
Review Comment:
The previous method wasn't clear enough, so I've replaced it with
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Support commit isolation level [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #3805:
URL: https://github.com/apache/paimon/pull/3805#issuecomment-2258411442
Thanks @BsoBird ! I need time to look this, next week will give your feedback.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org