You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by fanyon <gi...@git.apache.org> on 2017/05/03 06:43:53 UTC
[GitHub] flink pull request #3812: [FLINK-6346] Migrate from Java serialization for G...
GitHub user fanyon opened a pull request:
https://github.com/apache/flink/pull/3812
[FLINK-6346] Migrate from Java serialization for GenericWriteAheadSink's state
fix Migrate from Java serialization for GenericWriteAheadSink's state
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/fanyon/flink FLINK-6346
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3812.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 #3812
----
commit 0ccc5d8005828d892d16b5e1b32c93ce04607525
Author: mengji.fy <me...@taobao.com>
Date: 2017-05-03T06:06:21Z
[FLINK-6346] Migrate from Java serialization for GenericWriteAheadSink's state
commit 1a7d1539f7426320b218f8a6bdf2ebf155c5e434
Author: mengji.fy <me...@taobao.com>
Date: 2017-05-03T06:43:01Z
update import order
----
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by fanyon <gi...@git.apache.org>.
Github user fanyon closed the pull request at:
https://github.com/apache/flink/pull/3812
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/3812
```git push upstream -f xxx ``` DOES NOT WORK for squashed 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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by fanyon <gi...@git.apache.org>.
Github user fanyon commented on the issue:
https://github.com/apache/flink/pull/3812
@zhangminglei @zentol thanks for your suggestions, and I have updated the code.
As discussed in [https://github.com/apache/flink/pull/3750#issuecomment-298869900](url), it may not need to register two state like the current way, and older version of the data could be converted in some way, such as upgrade tools. I look forward to hearing your thoughts after you think about it later. @tzulitai
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3812
There is no doubt that you **can** modify the commits of an open PR. My point is that you **shouldn't** because it messes with the review.
This applies to both squashing commits and force pushing in general. The only exception is when you rebase the PR; but that should only be done if a rebase actually causes conflicts.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114483460
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -23,7 +23,9 @@
import java.util.Set;
import java.util.TreeSet;
import java.util.UUID;
+
--- End diff --
Hi, @fanyon Please care about these details, you shouldn not put a blank line here if I give advice. And as follows with the same rules. Thanks ~
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3812
For clarification: Commits should **not** be squashed once the PR has been opened. However, *before* opening a PR it would be good to squash these minor commits.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114501444
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -88,15 +93,23 @@ public void initializeState(StateInitializationContext context) throws Exception
Preconditions.checkState(this.checkpointedState == null,
"The reader state has already been initialized.");
- checkpointedState = context.getOperatorStateStore()
- .getSerializableListState("pending-checkpoints");
+ javaCheckpointedState = context.getOperatorStateStore().getSerializableListState(checkpointedStateDescriptor.getName());
--- End diff --
@zentol I just think he forgot it. So, like that.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114499181
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -88,15 +93,23 @@ public void initializeState(StateInitializationContext context) throws Exception
Preconditions.checkState(this.checkpointedState == null,
"The reader state has already been initialized.");
- checkpointedState = context.getOperatorStateStore()
- .getSerializableListState("pending-checkpoints");
+ javaCheckpointedState = context.getOperatorStateStore().getSerializableListState(checkpointedStateDescriptor.getName());
--- End diff --
@zentol I think he stores it in this field for later to write in ```snapshotState ```.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/3812
@zentol Sorry. I just drunk. My mean is we can use ```git rebase -i origin/master``` to combine lots of commit logs into one log and then use ```git push origin -f branch_issueId``` push to orgin server.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3812
@zhangminglei I don't understand what you're trying to say.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114497922
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -88,15 +93,23 @@ public void initializeState(StateInitializationContext context) throws Exception
Preconditions.checkState(this.checkpointedState == null,
"The reader state has already been initialized.");
- checkpointedState = context.getOperatorStateStore()
- .getSerializableListState("pending-checkpoints");
+ javaCheckpointedState = context.getOperatorStateStore().getSerializableListState(checkpointedStateDescriptor.getName());
+ checkpointedState = context.getOperatorStateStore().getListState(checkpointedStateDescriptor);
int subtaskIdx = getRuntimeContext().getIndexOfThisSubtask();
if (context.isRestored()) {
LOG.info("Restoring state for the GenericWriteAheadSink (taskIdx={}).", subtaskIdx);
- for (PendingCheckpoint pendingCheckpoint : checkpointedState.get()) {
- this.pendingCheckpoints.add(pendingCheckpoint);
+ try {
+ for (PendingCheckpoint pendingCheckpoint : javaCheckpointedState.get()) {
+ this.pendingCheckpoints.add(pendingCheckpoint);
+ }
+ } catch (Exception e) {
--- End diff --
What exceptions are we expecting here?
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/3812
@fanyon Another suggestion here. Make the lots of commit log into one log is nice.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114500437
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -88,15 +93,23 @@ public void initializeState(StateInitializationContext context) throws Exception
Preconditions.checkState(this.checkpointedState == null,
"The reader state has already been initialized.");
- checkpointedState = context.getOperatorStateStore()
- .getSerializableListState("pending-checkpoints");
+ javaCheckpointedState = context.getOperatorStateStore().getSerializableListState(checkpointedStateDescriptor.getName());
--- End diff --
It's never references anywhere outside this method though. `checkpointState` is, but not `javaCheckpointState`.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114500999
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -88,15 +93,23 @@ public void initializeState(StateInitializationContext context) throws Exception
Preconditions.checkState(this.checkpointedState == null,
"The reader state has already been initialized.");
- checkpointedState = context.getOperatorStateStore()
- .getSerializableListState("pending-checkpoints");
+ javaCheckpointedState = context.getOperatorStateStore().getSerializableListState(checkpointedStateDescriptor.getName());
--- End diff --
Just remove ```javaCheckpointedState``` filed is ok for this purpose.
---
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 #3812: [FLINK-6346] Migrate from Java serialization for GenericW...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3812
Hi, sorry for the late response here @fanyon.
I've left a comment on the umbrella JIRA on how we proceed with this. Could you take a look? Thanks :-D
---
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 #3812: [FLINK-6346] Migrate from Java serialization for G...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/3812#discussion_r114497478
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/operators/GenericWriteAheadSink.java ---
@@ -88,15 +93,23 @@ public void initializeState(StateInitializationContext context) throws Exception
Preconditions.checkState(this.checkpointedState == null,
"The reader state has already been initialized.");
- checkpointedState = context.getOperatorStateStore()
- .getSerializableListState("pending-checkpoints");
+ javaCheckpointedState = context.getOperatorStateStore().getSerializableListState(checkpointedStateDescriptor.getName());
--- End diff --
It's not necessary to store this in a field; it's only used within this method anyway.
---
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.
---