You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by shixiaogang <gi...@git.apache.org> on 2017/03/17 09:06:25 UTC
[GitHub] flink pull request #3558: [FLINK-6096][checkpoint] Refactor the migration of...
GitHub user shixiaogang opened a pull request:
https://github.com/apache/flink/pull/3558
[FLINK-6096][checkpoint] Refactor the migration of old-versioned savepoints
1. The migrated classes in `SavepointV0` are moved to the package `org.apache.flink.migration.v0`. In the future, each deprecated savepoint version will have its own migration package.
2. A mapping is deployed in `SavepointV0` to record all deprecated classes. `MigrationInstantiationUtil` now will use the mapping to correctly deserialized those deprecated classes.
3. Unused methods in migrated classes are removed.
4. The formats in old snapshots are added in the comments to help understand the restoring.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shixiaogang/flink flink-6096
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3558.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 #3558
----
commit b5f9d73703372c0d71da2c84cdd11dce16641b28
Author: xiaogang.sxg <xi...@alibaba-inc.com>
Date: 2017-03-17T08:56:00Z
Refactor the migration of old-versioned savepoints
----
---
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 #3558: [FLINK-6096][checkpoint] Refactor the migration of...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/3558#discussion_r108616525
--- Diff: flink-runtime/src/main/java/org/apache/flink/migration/v0/SavepointV0.java ---
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.migration.v0;
+
+import org.apache.flink.migration.v0.runtime.TaskStateV0;
+import org.apache.flink.runtime.checkpoint.TaskState;
+import org.apache.flink.runtime.checkpoint.savepoint.Savepoint;
+import org.apache.flink.util.Preconditions;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Savepoint version 0.
+ *
+ * <p>This format was introduced with Flink 1.1.0.
+ * <pre>
+ * checkpointId: long
+ * numTaskStates: int
+ * |----jobVertexID: long[2]
+ * |----parallelism: int
+ * |----numSubtaskStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----subtaskStateSize: long
+ * | |----subtaskStateDuration: long
+ * |----numKeyGroupStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----keyGroupStateSize: long
+ * | |----keyGroupStateDuration: long
+ * </pre>
+ */
+@Deprecated
+@SuppressWarnings("deprecation")
+public class SavepointV0 implements Savepoint {
+
+ /** The classes that are migrated in SavepointV0 */
+ public static final Map<String, String> MigrationMapping = new HashMap<String, String>() {{
+
+ /* migrated state descriptors */
+ put("org.apache.flink.api.common.state.StateDescriptor",
--- End diff --
I agree with your assessment about future changes. Typically, if we have a good testing for backwards compatibility in place, it will also reveal classes that are forgotten in the migration package through `ClassNotFoundException`s - but we should not only rely on that, I see this more like a second level safety-net.
Then I would suggest that we go for option b).
---
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 #3558: [FLINK-6096][checkpoint] Refactor the migration of...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/3558#discussion_r107908422
--- Diff: flink-runtime/src/main/java/org/apache/flink/migration/MigrationInstantiationUtil.java ---
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.migration;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.migration.v0.SavepointV0;
+import org.apache.flink.util.InstantiationUtil;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectStreamClass;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Utility class to deserialize legacy classes for migration.
+ */
+@PublicEvolving
+public final class MigrationInstantiationUtil {
+
+ private static final Map<Integer, Map<String, String>> MigrationMappings = new HashMap<Integer, Map<String, String>>() {{
--- End diff --
Minor code style comment: we use upper case and underscore for the names of all static final variables in Flink. For example: MIGRATION_MAPPINGS.
---
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 #3558: [FLINK-6096][checkpoint] Refactor the migration of...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/3558#discussion_r107908694
--- Diff: flink-runtime/src/main/java/org/apache/flink/migration/v0/SavepointV0.java ---
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.migration.v0;
+
+import org.apache.flink.migration.v0.runtime.TaskStateV0;
+import org.apache.flink.runtime.checkpoint.TaskState;
+import org.apache.flink.runtime.checkpoint.savepoint.Savepoint;
+import org.apache.flink.util.Preconditions;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Savepoint version 0.
+ *
+ * <p>This format was introduced with Flink 1.1.0.
+ * <pre>
+ * checkpointId: long
+ * numTaskStates: int
+ * |----jobVertexID: long[2]
+ * |----parallelism: int
+ * |----numSubtaskStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----subtaskStateSize: long
+ * | |----subtaskStateDuration: long
+ * |----numKeyGroupStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----keyGroupStateSize: long
+ * | |----keyGroupStateDuration: long
+ * </pre>
+ */
+@Deprecated
+@SuppressWarnings("deprecation")
+public class SavepointV0 implements Savepoint {
+
+ /** The classes that are migrated in SavepointV0 */
+ public static final Map<String, String> MigrationMapping = new HashMap<String, String>() {{
--- End diff --
Same comment about upper case variable names for static final variables.
---
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 #3558: [FLINK-6096][checkpoint] Refactor the migration of...
Posted by shixiaogang <gi...@git.apache.org>.
Github user shixiaogang commented on a diff in the pull request:
https://github.com/apache/flink/pull/3558#discussion_r108615377
--- Diff: flink-runtime/src/main/java/org/apache/flink/migration/v0/SavepointV0.java ---
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.migration.v0;
+
+import org.apache.flink.migration.v0.runtime.TaskStateV0;
+import org.apache.flink.runtime.checkpoint.TaskState;
+import org.apache.flink.runtime.checkpoint.savepoint.Savepoint;
+import org.apache.flink.util.Preconditions;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Savepoint version 0.
+ *
+ * <p>This format was introduced with Flink 1.1.0.
+ * <pre>
+ * checkpointId: long
+ * numTaskStates: int
+ * |----jobVertexID: long[2]
+ * |----parallelism: int
+ * |----numSubtaskStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----subtaskStateSize: long
+ * | |----subtaskStateDuration: long
+ * |----numKeyGroupStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----keyGroupStateSize: long
+ * | |----keyGroupStateDuration: long
+ * </pre>
+ */
+@Deprecated
+@SuppressWarnings("deprecation")
+public class SavepointV0 implements Savepoint {
+
+ /** The classes that are migrated in SavepointV0 */
+ public static final Map<String, String> MigrationMapping = new HashMap<String, String>() {{
+
+ /* migrated state descriptors */
+ put("org.apache.flink.api.common.state.StateDescriptor",
--- End diff --
@StefanRRichter +1 for option b)
I planned to use a tool to automatically create the mapping, but i found it very difficult to obtain the changes in the savepoints. It seems we must find all migrated classes manually.
I agree with you that the maintenance of the mapping is error-prone. The usage of a naming scheme convention will help us to avoid typo in new class names. But we still have to carefully find all classes that are changed in the savepoints. I think it's necessary for us. We must keep in mind what are changed in the savepoint format.
In my opinion, the number of migrated classes will be very small in the future. Most changes should happen in the serialization formats of actual states (e.g., the snapshots of keyed states and timer services) instead of the state handles. Therefore, there will be few efforts needed to maintain the mapping.
---
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 #3558: [FLINK-6096][checkpoint] Refactor the migration of...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/3558#discussion_r108133263
--- Diff: flink-runtime/src/main/java/org/apache/flink/migration/v0/SavepointV0.java ---
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.migration.v0;
+
+import org.apache.flink.migration.v0.runtime.TaskStateV0;
+import org.apache.flink.runtime.checkpoint.TaskState;
+import org.apache.flink.runtime.checkpoint.savepoint.Savepoint;
+import org.apache.flink.util.Preconditions;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Savepoint version 0.
+ *
+ * <p>This format was introduced with Flink 1.1.0.
+ * <pre>
+ * checkpointId: long
+ * numTaskStates: int
+ * |----jobVertexID: long[2]
+ * |----parallelism: int
+ * |----numSubtaskStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----subtaskStateSize: long
+ * | |----subtaskStateDuration: long
+ * |----numKeyGroupStates: int
+ * | |----subtaskIndex: int
+ * | |----serializedValueLength: int
+ * | |----serializedValue: byte[] (null if serializedValueLength is -1)
+ * | |----keyGroupStateSize: long
+ * | |----keyGroupStateDuration: long
+ * </pre>
+ */
+@Deprecated
+@SuppressWarnings("deprecation")
+public class SavepointV0 implements Savepoint {
+
+ /** The classes that are migrated in SavepointV0 */
+ public static final Map<String, String> MigrationMapping = new HashMap<String, String>() {{
+
+ /* migrated state descriptors */
+ put("org.apache.flink.api.common.state.StateDescriptor",
--- End diff --
I wonder if it is a good idea to make the mapping as explicit as this. While this offers more flexibility than the previous approach, we also need to maintain this mapping by hand. This can be error prone, in case a class is forgotten or there is a typo. Would it make sense to either a) create the string mappings in this map automatically through code that explores classes in the migration package or b) keep a naming scheme convention, similar to before but with the version attached in the package in class name? Then we could use this map to only contain mappings that we want to be explicitly different from to convention (is there a case for this?). My feeling is that option b) might be better. What do you think?
---
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.
---