You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sihuazhou <gi...@git.apache.org> on 2018/03/15 16:13:13 UTC

[GitHub] flink pull request #5705: [FLINK-8968][state]Fix native resource leak caused...

GitHub user sihuazhou opened a pull request:

    https://github.com/apache/flink/pull/5705

    [FLINK-8968][state]Fix native resource leak caused by ReadOptions

    ## What is the purpose of the change
    
    This PR fixes (FLINK-8968)[https://issues.apache.org/jira/browse/FLINK-8968] Pull the creation of `ReadOptions` out of loop to avoid native resource leak in Full checkpoint.
    
    ## Brief change log
    
    - pull the creation of `ReadOptions` out of loop in `RocksDBFullSnapshotOperation.writeKVStateMetaData()`
    
    ## Verifying this change
    
    This change is a trivial rework / code cleanup without any test coverage.
    
    ## 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)`: ( no)
      - The serializers: (no)
      - The runtime per-record code paths (performance sensitive): (no)
      - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes)
      - The S3 file system connector: (no)
    
    ## Documentation
    no

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sihuazhou/flink fix_readOptions_leak

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/flink/pull/5705.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 #5705
    
----
commit f1cd2e7f6962e3307e842712c68200362bec61d7
Author: sihuazhou <su...@...>
Date:   2018-03-15T15:57:11Z

    Poll the creation of readOptions out of loop to avoid native resource leak.

----


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    I made a fixup for some additional minor bug in `RocksDBStateBackendConfigTest.java`.


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    You can cover it here and I will review it again 👍 


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    @StefanRRichter I have updated the code, could you please have a look...


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    @StefanRRichter Sorry for pinging you again in this PR, but I found there maybe be another issue for `full checkpoint`. It's about the `kvStateInformation`, we should create a deep copy of `kvStateInformation` when taking snapshot, currently we use the code `this.kvStateInformationCopy = new ArrayList<>(stateBackend.kvStateInformation.values());`, but this is not a deep copy indeed because the `Serializers` in `RegisteredKeyedBackendStateMetaInfo` maybe not stateless. If you also think this is a problem, instead of make a new PR for that I'd like to change the code to cover it in this PR, WDYT?


---

[GitHub] flink issue #5705: [FLINK-8699][FLINK-8968][state]Fix native resource leak c...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    Could anyone have a look at this PR?


---

[GitHub] flink pull request #5705: [FLINK-8699][FLINK-8968][state]Fix native resource...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/flink/pull/5705


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    LGTM 👍 


---

[GitHub] flink issue #5705: [FLINK-8699][FLINK-8968][state]Fix native resource leak c...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    Hi, @StefanRRichter thanks a lot.


---

[GitHub] flink issue #5705: [FLINK-8699][FLINK-8968][state]Fix native resource leak c...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    Hi @tillrohrmann thanks for your reply, and god bless Stefan, hope he will get well soon...


---

[GitHub] flink issue #5705: [FLINK-8699][FLINK-8968][state]Fix native resource leak c...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    Hi @sihuazhou, Stefan is currently sick but he should come back in a bit. Since he already reviewed parts of the PR, I would suggest to wait for his return. If he should not be back by next week, I'll take over.


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    @StefanRRichter Could you please have a look at this? 


---

[GitHub] flink issue #5705: [FLINK-8968][state]Fix native resource leak caused by Rea...

Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    Hi @tillrohrmann could you please have a look at this? This PR covers multi bugs that should be addressed...I think maybe stefan is too busy with the testing works on 1.5 currently...


---

[GitHub] flink issue #5705: [FLINK-8699][FLINK-8968][state]Fix native resource leak c...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/5705
  
    Thanks for the fixes. LGTM 👍 Will merge this. Sorry that this took so long.


---