You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/08/02 10:37:52 UTC
[GitHub] [flink] Myasuka opened a new pull request, #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Myasuka opened a new pull request, #20421:
URL: https://github.com/apache/flink/pull/20421
## What is the purpose of the change
This PR deprecates the ambiguous StateTTLConfig#cleanFullSnapshot API with the newly introduced `#cleanupOnFullScanSnapshot` API.
## Brief change log
1. Deprecate the StateTTLConfig#cleanFullSnapshot API
2. Introduce StateTTLConfig#cleanupOnFullScanSnapshot API
3. Add docs to describe the difference.
## Verifying this change
This change is already covered by existing tests.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: yes
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? no
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940412527
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (including the canonical savepoint,
+or the full/incremental checkpoint of hashmap state-backend, or the full checkpoint of RocksDB state-backend), which
Review Comment:
I think telling users with a direct explanation of what this API can take effect could be more accurate. And I believe these descriptions would change if we apply new implementations for different state-backend, e.g FLINK-28699 or FLIP-151.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940964862
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (means the
+canonical savepoint of all state-backends, or the checkpoint/native-savepoint of hashmap state-backend.), which
Review Comment:
Hmm..I was thinking that RocksDB non-incremental checkpoints also support this TTL mode, don't they?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941260334
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I mean that **just** enumarating the cases is enough to clarify this TTL policy;
But it's not enough to explain what the "full scan snapshot" is.
So I think either the definition of "full scan snapshot" should be given; or the "full scan snapshot" term should be removed.
Removing seems preferrable lest overload users with unnecessary information.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-2768299963
This PR has been closed since it has not had any activity in 120 days.
If you feel like this was a mistake, or you would like to continue working on it,
please feel free to re-open the PR and ask for a review.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1209303130
I'm [concerned](https://github.com/apache/flink/pull/20421#discussion_r941260334) about introducing the unnecessary term (from my perspective), but I wouldn't mind merging this PR as it is.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1209488340
> I still have the same https://github.com/apache/flink/pull/20421#discussion_r940348087.
Probably someone else would disagree with me and approve the PR, I'd be absolutely fine with that.
It was too close to the feature freeze date to ask someone else to review, I think we should make the code clean instead of rushing the changes. I would not drive this PR merged in 1.16 with existing concerns.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-2592596999
This PR is being marked as stale since it has not had any activity in the last 180 days.
If you would like to keep this PR alive, please leave a comment asking for a review.
If the PR has merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out to the
[community](https://flink.apache.org/what-is-flink/community/).
<p>
If this PR is no longer valid or desired, please feel free to close it.
If no activity occurs in the next 90 days, it will be automatically closed.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941289259
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I just feel that the changes would be a bit too large if dropping the term since we still have them in the doc of the previous version. Moreover, we still support this API. That's why I prefer to still leave this term there.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941283955
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I'm afraid we'll never know that :)
And the term is also introduced into the API.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941370321
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I'm afraid that changing the API is not allowed during the release testing phase (which makes a lot of sense).
So it won't be possible to revert this 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1209401763
Would you please approve this PR @rkhachatryan if no more concerns?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940868846
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (including the canonical savepoint,
+or the full/incremental checkpoint of hashmap state-backend, or the full checkpoint of RocksDB state-backend), which
Review Comment:
Got it, I will refactor these descriptions.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940348087
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I'm not sure whether adding `scan` here and elsewhere adds more clarity because it is not explained anywhere what a "full-scan snapshot" is.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] fredia commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
fredia commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941966732
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (means the
+canonical savepoint of all state-backends, or the checkpoint/native-savepoint of hashmap state-backend.), which
Review Comment:
Will RocksDB non-incremental checkpoints support `cleanupInRocksdbCompactFilter` After FLINK-28699？
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940989293
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (means the
+canonical savepoint of all state-backends, or the checkpoint/native-savepoint of hashmap state-backend.), which
Review Comment:
After FLINK-28699, it was not true.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1202327776
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "0f8728ab040b721df06d2a569019751da57f416d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "0f8728ab040b721df06d2a569019751da57f416d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 0f8728ab040b721df06d2a569019751da57f416d UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941327527
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
By term I [mean](https://github.com/apache/flink/pull/20421#discussion_r940348087) `scan`, which is introduced by **this** PR.
And I think it's better to keep unresolved conversations unresolved, even if the PR is going to be merged in the 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1209288144
@rkhachatryan do you have more concerns? My private build is successful: https://dev.azure.com/myasuka/flink/_build/results?buildId=429&view=results.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940413545
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
That's why I explained what kind of snapshot is a full scan snapshot below.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940512547
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (including the canonical savepoint,
+or the full/incremental checkpoint of hashmap state-backend, or the full checkpoint of RocksDB state-backend), which
Review Comment:
Then how about listing the supported combinations explicitly under "supported"?
```
Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot.
This is only supported for canonical savepoints and full checkpoints with HashMap and RocksDB state backends.
```
From the current wording (`including ...`) it's not clear to me whether the list is exhaustive or not.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940503414
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
Do you mean this statement:
```
Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (including the canonical savepoint,
or the full/incremental checkpoint of hashmap state-backend, or the full checkpoint of RocksDB state-backend), which will reduce its size.
```
?
It only enumerates the cases, but doesn't explain the "full scan".
And thtat's probably right, I don't think we should overload the user with extra terms and their details.
Besides that, state backends might implement it differently.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941343413
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I see. Since any changes to public API should be considered new features. I will first merge this PR today just before the feature freeze deadline. We can change related docs or even revert this change during the release testing phase.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r940348087
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I'm not sure whether adding `scan` everywhere adds more clarity.
because it is not explained anywhere what is a "full-scan snapshot".
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (including the canonical savepoint,
+or the full/incremental checkpoint of hashmap state-backend, or the full checkpoint of RocksDB state-backend), which
Review Comment:
IIUC, the intention is to list all the cases except RocksDB native savepoints and incremental checkpoints, right? Why not to do that explicitly?
Listing all state backends and their modes here might require changes in the future.
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (including the canonical savepoint,
+or the full/incremental checkpoint of hashmap state-backend, or the full checkpoint of RocksDB state-backend), which
Review Comment:
IIUC, the intention is to list all the cases except RocksDB native savepoints and incremental checkpoints, right? Why not to do that explicitly?
Listing all state backends and their modes here might require changes in the future.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API [flink]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
URL: https://github.com/apache/flink/pull/20421
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1207198853
@pnowojski , @rkhachatryan could you please take a look at this?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941370321
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I'm afraid that changing the API is not allowed during the release testing phase, so it won't be possible to revert this 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on PR #20421:
URL: https://github.com/apache/flink/pull/20421#issuecomment-1209419220
I still have the same [concern](https://github.com/apache/flink/pull/20421#discussion_r940348087).
Probably someone else would disagree with me and approve the PR, I'd be absolutely fine with 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941969248
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
-Additionally, you can activate the cleanup at the moment of taking the full state snapshot which
+Additionally, you can activate the cleanup at the moment of taking the full scan state snapshot (means the
+canonical savepoint of all state-backends, or the checkpoint/native-savepoint of hashmap state-backend.), which
Review Comment:
@fredia sure, cleanup with compaction filter is only related with RocksDB's background compaction and has no direct relationship with what kind of snapshot.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941081228
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
I think it's good to show all cases here.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #20421: [FLINK-28732][state] Deprecate ambiguous StateTTLConfig#cleanFullSnapshot API
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #20421:
URL: https://github.com/apache/flink/pull/20421#discussion_r941272331
##########
docs/content/docs/dev/datastream/fault-tolerance/state.md:
##########
@@ -474,9 +474,10 @@ ttl_config = StateTtlConfig \
For more fine-grained control over some special cleanup in background, you can configure it separately as described below.
Currently, heap state backend relies on incremental cleanup and RocksDB backend uses compaction filter for background cleanup.
-##### Cleanup in full snapshot
+##### Cleanup in full scan snapshot
Review Comment:
Hmm... we can refactor the docs later if this will really bother users.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org