You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by rmetzger <gi...@git.apache.org> on 2015/05/14 17:14:13 UTC
[GitHub] flink pull request: [FLINK-2004] Fix memory leak in presense of fa...
GitHub user rmetzger opened a pull request:
https://github.com/apache/flink/pull/674
[FLINK-2004] Fix memory leak in presense of failed checkpoints for Kafka Source
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rmetzger/flink flink2004
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/674.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 #674
----
commit 27f11822b7db2716f3484def8ad350eb7e0b0893
Author: Robert Metzger <rm...@apache.org>
Date: 2015-05-14T09:45:30Z
[FLINK-2004] Fix memory leak in presense of failed checkpoints in Kafka source
commit 36cb4758c200713a97858989ac73f117186ed9dc
Author: Robert Metzger <rm...@apache.org>
Date: 2015-05-14T13:57:18Z
unused imports
----
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/674#issuecomment-105846281
What is the status of this pull request? Any pending changes, or is it blocked by anything?
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/674#issuecomment-105926083
Sorry for not reacting. I was busy with other stuff but I'll now address your comments.
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/674#issuecomment-105951860
I have those unit test style tests all in the KafkaITCase because they depend on the testing clusters started for the test.
They all need at least a running Zookeeper instance.
I can of course put them into a different class, but this will further slow down our tests because we spend more time starting and stopping zookeeper.
I've reworked the restore to reflect the open() / restoreState() order.
The PR has been updated.
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/674#discussion_r30787124
--- Diff: flink-staging/flink-streaming/flink-streaming-connectors/src/main/java/org/apache/flink/streaming/connectors/kafka/api/persistent/PersistentKafkaSource.java ---
@@ -236,13 +246,22 @@ public void restoreState(long[] state) {
@Override
public void commitCheckpoint(long checkpointId) {
LOG.info("Commit checkpoint {}", checkpointId);
- long[] checkpointOffsets = pendingCheckpoints.remove(checkpointId);
- if(checkpointOffsets == null) {
+ final int posInMap = pendingCheckpoints.indexOf(checkpointId);
+ if(posInMap == -1) {
LOG.warn("Unable to find pending checkpoint for id {}", checkpointId);
return;
}
+
+ long[] checkpointOffsets = (long[]) pendingCheckpoints.remove(posInMap);
LOG.info("Got corresponding offsets {}", Arrays.toString(checkpointOffsets));
--- End diff --
I think this log message can be improved. To something like "Committing offsets {} to Kafka."
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/674
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/674#issuecomment-104206104
Looks good. Some of the tests look like unit-style tests that are sort of lightweight.
May be good to pull those out in to a `PersistentKafkaSourceTest`-
Otherwise, +1 to merge
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/674#discussion_r30787324
--- Diff: flink-staging/flink-streaming/flink-streaming-connectors/src/main/java/org/apache/flink/streaming/connectors/kafka/api/persistent/PersistentKafkaSource.java ---
@@ -225,7 +226,16 @@ public void close() {
@Override
public void restoreState(long[] state) {
- // we maintain the offsets in Kafka, so nothing to do.
+ if(lastOffsets == null) {
+ LOG.warn("Restore state called before open() has been called");
+ return;
+ }
+ LOG.info("Restoring state to {}", Arrays.toString(state));
+ if(lastOffsets.length != state.length) {
--- End diff --
How about sanity checking before logging that things are going to happen? Usually gives better logging insights...
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/674#discussion_r30787289
--- Diff: flink-staging/flink-streaming/flink-streaming-connectors/src/main/java/org/apache/flink/streaming/connectors/kafka/api/persistent/PersistentKafkaSource.java ---
@@ -225,7 +226,16 @@ public void close() {
@Override
public void restoreState(long[] state) {
- // we maintain the offsets in Kafka, so nothing to do.
+ if(lastOffsets == null) {
+ LOG.warn("Restore state called before open() has been called");
--- End diff --
Actually, restore state is always called before `open()`, as far as I know.
---
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: [FLINK-2004] Fix memory leak in presence of fa...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/674#issuecomment-107392647
Looks good!
Will merge this..
---
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.
---