You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "Aitozi (via GitHub)" <gi...@apache.org> on 2024/05/22 11:43:56 UTC
[PR] [core] Introduce ChangelogDeletion to Expiration [paimon]
Aitozi opened a new pull request, #3383:
URL: https://github.com/apache/paimon/pull/3383
<!-- Please specify the module before the PR name: [core] ... or [flink] ... -->
### Purpose
This PR is the second step of https://github.com/apache/paimon/pull/3178 to simply the clean up logic of snapshot and changelog
<!-- What is the purpose of the change -->
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1611123235
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
Review Comment:
For example, `SnapshotDeletion` will use this to determine whether to clean changelog manifests. It will also be used in `ChangelogDeletion` in following 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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #3383:
URL: https://github.com/apache/paimon/pull/3383
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1611047076
##########
paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java:
##########
@@ -309,23 +308,14 @@ public TableCommitImpl newCommit(String commitUser) {
if (!options.writeOnly()) {
boolean changelogDecoupled = options.changelogLifecycleDecoupled();
ExpireSnapshots expireChangelog =
- newExpireChangelog()
- .maxDeletes(options.snapshotExpireLimit())
- .retainMin(options.changelogNumRetainMin())
- .retainMax(options.changelogNumRetainMax());
+ newExpireChangelog().config(options.expireConfig().build());
Review Comment:
reuse `options.expireConfig().build()`.
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
Review Comment:
changelogDecoupled who use this?
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
+
+ /** Used to record which tag is cached in tagged snapshots list. */
+ private int cachedTagIndex = -1;
+
+ /** Used to cache data files used by current tag. */
+ private final Map<BinaryRow, Map<Integer, Set<String>>> cachedTagDataFiles = new HashMap<>();
Review Comment:
maybe we should introduce a `ExpireDeletion`? `TagDeletion` will not use this field.
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1613228788
##########
paimon-core/src/main/java/org/apache/paimon/operation/TagDeletion.java:
##########
@@ -88,15 +91,21 @@ public void cleanUnusedManifests(Snapshot taggedSnapshot, Set<String> skippingSe
cleanUnusedManifests(taggedSnapshot, skippingSet, false);
}
- public Predicate<ManifestEntry> dataFileSkipper(Snapshot fromSnapshot) throws Exception {
- return dataFileSkipper(Collections.singletonList(fromSnapshot));
- }
+ protected Collection<ManifestEntry> readMergedDataFiles(Snapshot snapshot) throws IOException {
Review Comment:
Duplicate codes?
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1614691375
##########
paimon-core/src/main/java/org/apache/paimon/operation/TagDeletion.java:
##########
@@ -88,15 +91,21 @@ public void cleanUnusedManifests(Snapshot taggedSnapshot, Set<String> skippingSe
cleanUnusedManifests(taggedSnapshot, skippingSet, false);
}
- public Predicate<ManifestEntry> dataFileSkipper(Snapshot fromSnapshot) throws Exception {
- return dataFileSkipper(Collections.singletonList(fromSnapshot));
- }
+ protected Collection<ManifestEntry> readMergedDataFiles(Snapshot snapshot) throws IOException {
Review Comment:
The latest commit is reverted, please take a look again when you are free.
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1611206608
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
+
+ /** Used to record which tag is cached in tagged snapshots list. */
+ private int cachedTagIndex = -1;
+
+ /** Used to cache data files used by current tag. */
+ private final Map<BinaryRow, Map<Integer, Set<String>>> cachedTagDataFiles = new HashMap<>();
Review Comment:
It only be cached when accessed, So in `TagDeletion` it will not be populated
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1611138944
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
Review Comment:
but there is no assignment for it.
maybe you need to add it in next 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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1613239302
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
+
+ /** Used to record which tag is cached in tagged snapshots list. */
+ private int cachedTagIndex = -1;
+
+ /** Used to cache data files used by current tag. */
+ private final Map<BinaryRow, Map<Integer, Set<String>>> cachedTagDataFiles = new HashMap<>();
Review Comment:
My bad. TagDeletion will use skipper too.
--
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] Introduce ChangelogDeletion to Expiration [paimon]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #3383:
URL: https://github.com/apache/paimon/pull/3383#discussion_r1611202391
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileDeletionBase.java:
##########
@@ -70,6 +73,13 @@ public abstract class FileDeletionBase {
protected final Map<BinaryRow, Set<Integer>> deletionBuckets;
protected final Executor ioExecutor;
+ protected boolean changelogDecoupled;
Review Comment:
There is `setChangelogDecoupled` . I feel difficult to make this field `final`, so I add a set method for 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