You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by StefanRRichter <gi...@git.apache.org> on 2017/08/16 15:19:45 UTC
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
GitHub user StefanRRichter opened a pull request:
https://github.com/apache/flink/pull/4550
[FLINK-7461] Remove Backwards compatibility with <= Flink 1.1
## Brief change log
This PR removes backwards compatibility with Flink <= 1.1 from the code. In particular, we can remove many special cases, such as the non-partitionable state that was created by the `Checkpointed` interface (replaced by `CheckpointedFunction` in 1.2).
We also drop aligned windows, which have already been a "hidden feature" since 1.2, because they still rely on the outdated `Checkpointed` interface.
The `Checkpointed` and `CheckpointedRestoring` interfaces are also removed and all related test cases have either been deleted or adapted.
## Verifying this change
This change is already covered by existing tests, such as *(please describe 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: (yes, SavepointSerializer)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes)
## Documentation
- Does this pull request introduce a new feature? (no)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/StefanRRichter/flink drop-1.1-compatibility
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4550.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4550
----
commit 50bbc938f3484e8a0d81511db719ebe296b8df94
Author: Stefan Richter <s....@data-artisans.com>
Date: 2017-08-14T12:01:03Z
[FLINK-7460] [state backends] Close all ColumnFamilyHandles when restoring from rescaled incremental checkpoints
commit 95e44099784c9deaf2ca422b8dfc11c3d67d7f82
Author: Stefan Richter <s....@data-artisans.com>
Date: 2017-08-14T12:02:37Z
[FLINK-7461] Remove Backwards compatibility with <= Flink 1.1
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4550
CC @aljoscha @StephanEwen ; the relevant commit is only 95e44099784c9deaf2ca422b8dfc11c3d67d7f82 .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/4550
Just wanted to add to what @kl0u said that if we remove the compatibility with <= 1.2 of CEP library there will be lot more code that will be no longer used than just that in this PR. E.g.
- `NFA#migrateNFA`
- `NFACompiler#migrateGraph`
- `SharedBuffer#migrateSharedBuffer`
- `SharedBuffer#readObject`
just to name a few. I think it would be really important to create a following JIRA to introduce those changes in CEP, if we proceed with this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133493395
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/StateUtil.java ---
@@ -49,27 +49,8 @@ public static long getStateSize(StateObject handle) {
* @throws Exception exception that is a collection of all suppressed exceptions that were caught during iteration
*/
public static void bestEffortDiscardAllStateObjects(
- Iterable<? extends StateObject> handlesToDiscard) throws Exception {
-
- if (handlesToDiscard != null) {
- Exception exception = null;
-
- for (StateObject state : handlesToDiscard) {
-
- if (state != null) {
- try {
- state.discardState();
- }
- catch (Exception ex) {
- exception = ExceptionUtils.firstOrSuppressed(ex, exception);
- }
- }
- }
-
- if (exception != null) {
- throw exception;
- }
- }
+ Iterable<? extends StateObject> handlesToDiscard) throws Exception {
+ LambdaUtil.applyToAllWhileSuppressingExceptions(handlesToDiscard, StateObject::discardState);
--- End diff --
Is this change really necessary?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/4550
@StefanRRichter I am ok either with you removing it in this PR or with creating a separate PR, that e.g. I could work on. Just wanted to make sure we will not miss it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r135091891
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/FlinkKinesisConsumerMigrationTest.java ---
@@ -1,149 +0,0 @@
-/*
--- End diff --
I discussed with @aljoscha that this class can be resurrected when merging #4565
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/4550
I agree that this is a change that we agreed upon. My only point is that we have to communicate it also in the ML. Probably with a thread that points to this JIRA and the PR and mentions this issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r134971518
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/FlinkKinesisConsumerMigrationTest.java ---
@@ -1,149 +0,0 @@
-/*
--- End diff --
We can't remove this because there is another PR that updates this for Flink 1.2 and 1.3: #4565
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4550
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133663601
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackendTest.java ---
@@ -122,6 +124,22 @@ protected RocksDBStateBackend getStateBackend() throws IOException {
return backend;
}
+ // small safety net for instance cleanups, so that no native objects are left
--- End diff --
This change actually belongs to the previous commit, so I will put it back there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4550
Thanks for the review @aljoscha ! Will merge now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4550
I have pushed an updated version with the changes that @zentol suggested and the additional cleanups suggested by @dawidwys.
@dawidwys I removed the code that you mentioned, but it is still possible that there more dead code in CEP (or even other places).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133490230
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/OperatorSubtaskState.java ---
@@ -43,17 +40,17 @@
* This class encapsulates the state for one parallel instance of an operator. The complete state of a (logical)
* operator (e.g. a flatmap operator) consists of the union of all {@link OperatorSubtaskState}s from all
* parallel tasks that physically execute parallelized, physical instances of the operator.
- *
+ * <p>
--- End diff --
this should be an empty line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133492522
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackendTest.java ---
@@ -122,6 +124,22 @@ protected RocksDBStateBackend getStateBackend() throws IOException {
return backend;
}
+ // small safety net for instance cleanups, so that no native objects are left
--- End diff --
are these changes necessary for this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by kl0u <gi...@git.apache.org>.
Github user kl0u commented on the issue:
https://github.com/apache/flink/pull/4550
From a brief check, I see that this also removed backwards compatibility also for 1.2 for the CEP library. The reason is that the CEP library in Flink 1.2 was the same as in Flink 1.1 (no upgrade) and so it was using the old state abstractions.
Given that in the ML we discussed about dropping compatibility with 1.1 and agreed on that, a library should not affect that development, but there must be an explicit discussion in the ML for this specific matter before merging it. Even if this just means just saying that you have to go through 1.3 for CEP if you want to migrate from 1.2 to 1.4.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133490130
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/OperatorSubtaskState.java ---
@@ -228,12 +195,11 @@ public StreamStateHandle getLegacyOperatorState() {
public void discardState() {
try {
List<StateObject> toDispose =
- new ArrayList<>(1 +
+ new ArrayList<>(
managedOperatorState.size() +
- rawOperatorState.size() +
- managedKeyedState.size() +
- rawKeyedState.size());
- toDispose.add(legacyOperatorState);
+ rawOperatorState.size() +
--- End diff --
odd indentation
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133663689
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/StateUtil.java ---
@@ -49,27 +49,8 @@ public static long getStateSize(StateObject handle) {
* @throws Exception exception that is a collection of all suppressed exceptions that were caught during iteration
*/
public static void bestEffortDiscardAllStateObjects(
- Iterable<? extends StateObject> handlesToDiscard) throws Exception {
-
- if (handlesToDiscard != null) {
- Exception exception = null;
-
- for (StateObject state : handlesToDiscard) {
-
- if (state != null) {
- try {
- state.discardState();
- }
- catch (Exception ex) {
- exception = ExceptionUtils.firstOrSuppressed(ex, exception);
- }
- }
- }
-
- if (exception != null) {
- throw exception;
- }
- }
+ Iterable<? extends StateObject> handlesToDiscard) throws Exception {
+ LambdaUtil.applyToAllWhileSuppressingExceptions(handlesToDiscard, StateObject::discardState);
--- End diff --
Not strictly, but I would like to keep it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4550
@dawidwys It is of course possible that I missed some dead code between. I can also just remove the parts you suggested for this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133663458
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/testutils/ExactlyOnceValidatingConsumerThread.java ---
@@ -95,7 +94,7 @@ public void run() {
return new Thread(exactlyOnceValidationConsumer);
}
- private static class ExactlyOnceValidatingMapper implements FlatMapFunction<String, String>, Checkpointed<BitSet> {
+ private static class ExactlyOnceValidatingMapper implements FlatMapFunction<String, String> {
--- End diff --
Ok, I will replace the logic by `ListCheckpointed`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4550: [FLINK-7461] Remove Backwards compatibility with <= Flink...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4550
Yes, that is true, but I think it should because otherwise there is still no way to remove the `Checkpointed` / legacy state related code paths.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4550: [FLINK-7461] Remove Backwards compatibility with <...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4550#discussion_r133490898
--- Diff: flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/testutils/ExactlyOnceValidatingConsumerThread.java ---
@@ -95,7 +94,7 @@ public void run() {
return new Thread(exactlyOnceValidationConsumer);
}
- private static class ExactlyOnceValidatingMapper implements FlatMapFunction<String, String>, Checkpointed<BitSet> {
+ private static class ExactlyOnceValidatingMapper implements FlatMapFunction<String, String> {
--- End diff --
This _should_ cause the test to fail as far as i can tell.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---