[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16365366#comment-16365366 ] ASF GitHub Bot commented on FLINK-8212: --- Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/5458 > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Assignee: mingleizhang >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364076#comment-16364076 ] ASF GitHub Bot commented on FLINK-8212: --- Github user zhangminglei commented on a diff in the pull request: https://github.com/apache/flink/pull/5458#discussion_r168180788 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerServices.java --- @@ -530,10 +538,12 @@ public static long calculateNetworkBufferMemory(long totalJavaMemorySize, Config * . * * @param tmConfig task manager services configuration object +* @param maxJvmHeapMemory the maximum JVM heap size * * @return memory to use for network buffers (in bytes) */ - public static long calculateNetworkBufferMemory(TaskManagerServicesConfiguration tmConfig) { + public static long calculateNetworkBufferMemory(TaskManagerServicesConfiguration tmConfig, + long maxJvmHeapMemory) { --- End diff -- Will fix ~ > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Assignee: mingleizhang >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364074#comment-16364074 ] ASF GitHub Bot commented on FLINK-8212: --- Github user zhangminglei commented on a diff in the pull request: https://github.com/apache/flink/pull/5458#discussion_r168180756 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerServices.java --- @@ -270,10 +273,14 @@ public static TaskManagerServices fromConfiguration( * Creates a {@link MemoryManager} from the given {@link TaskManagerServicesConfiguration}. * * @param taskManagerServicesConfiguration to create the memory manager from +* @param freeHeapMemoryWithDefrag an estimate of the size of the free heap memory +* @param maxJvmHeapMemory the maximum JVM heap size * @return Memory manager * @throws Exception */ - private static MemoryManager createMemoryManager(TaskManagerServicesConfiguration taskManagerServicesConfiguration) throws Exception { + private static MemoryManager createMemoryManager(TaskManagerServicesConfiguration taskManagerServicesConfiguration, + long freeHeapMemoryWithDefrag, + long maxJvmHeapMemory) throws Exception { --- End diff -- Will fix ~ > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Assignee: mingleizhang >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364011#comment-16364011 ] ASF GitHub Bot commented on FLINK-8212: --- Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/5458#discussion_r168171985 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerServices.java --- @@ -270,10 +273,14 @@ public static TaskManagerServices fromConfiguration( * Creates a {@link MemoryManager} from the given {@link TaskManagerServicesConfiguration}. * * @param taskManagerServicesConfiguration to create the memory manager from +* @param freeHeapMemoryWithDefrag an estimate of the size of the free heap memory +* @param maxJvmHeapMemory the maximum JVM heap size * @return Memory manager * @throws Exception */ - private static MemoryManager createMemoryManager(TaskManagerServicesConfiguration taskManagerServicesConfiguration) throws Exception { + private static MemoryManager createMemoryManager(TaskManagerServicesConfiguration taskManagerServicesConfiguration, + long freeHeapMemoryWithDefrag, + long maxJvmHeapMemory) throws Exception { --- End diff -- The convention is to break parameter lists like ``` public void a( A a, B b) { // foobar } ``` > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16364012#comment-16364012 ] ASF GitHub Bot commented on FLINK-8212: --- Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/5458#discussion_r168172098 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskManagerServices.java --- @@ -530,10 +538,12 @@ public static long calculateNetworkBufferMemory(long totalJavaMemorySize, Config * . * * @param tmConfig task manager services configuration object +* @param maxJvmHeapMemory the maximum JVM heap size * * @return memory to use for network buffers (in bytes) */ - public static long calculateNetworkBufferMemory(TaskManagerServicesConfiguration tmConfig) { + public static long calculateNetworkBufferMemory(TaskManagerServicesConfiguration tmConfig, + long maxJvmHeapMemory) { --- End diff -- Same here with the parameters > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16363878#comment-16363878 ] ASF GitHub Bot commented on FLINK-8212: --- Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/5458 @tillrohrmann I updated the code. Could you take a look ? Thanks ~ > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16363685#comment-16363685 ] mingleizhang commented on FLINK-8212: - Thanks [~till.rohrmann] I will correct the code again. > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16360577#comment-16360577 ] Till Rohrmann commented on FLINK-8212: -- Hi [~mingleizhang], I think the proper way to solve this problem is to pass in the required information to the {{fromConfiguration}} method. > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16360482#comment-16360482 ] ASF GitHub Bot commented on FLINK-8212: --- GitHub user zhangminglei opened a pull request: https://github.com/apache/flink/pull/5458 [FLINK-8212] [network] Pull EnvironmentInformation out of TaskManager… ## What is the purpose of the change Pull EnvironmentInformation out of TaskManagerServices ## Brief change log Add inner class ```TaskExecutorEnvironmentInformation``` that can give access to the execution environment of the JVM. ## Verifying this change Run ```NetworkBufferCalculationTest``` is OKay. ## 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): ( don't know) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: ( no ) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? ( no) - If yes, how is the feature documented? (not documented) You can merge this pull request into a Git repository by running: $ git pull https://github.com/zhangminglei/flink flink-8212 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/5458.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 #5458 commit a4792758d3620359f9f42c3bcffbe7578bba4048 Author: zhangmingleiDate: 2018-02-12T09:21:42Z [FLINK-8212] [network] Pull EnvironmentInformation out of TaskManagerServices > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16359861#comment-16359861 ] mingleizhang commented on FLINK-8212: - I can directly move those methods to {{TaskManagerServices}} class and then refactor {{NetworkBufferCalculationTest}}. But I am not sure whether it is a better way to do that. > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-8212) Pull EnvironmentInformation out of TaskManagerServices
[ https://issues.apache.org/jira/browse/FLINK-8212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16359833#comment-16359833 ] mingleizhang commented on FLINK-8212: - Hi, [~till.rohrmann] I have a question. If we do not use {{EnvironmentInformation}} to get {{getSizeOfFreeHeapMemoryWithDefrag}} and {{getMaxJvmHeapMemory}}. Is there another way to get those two guys information ? Thanks. > Pull EnvironmentInformation out of TaskManagerServices > -- > > Key: FLINK-8212 > URL: https://issues.apache.org/jira/browse/FLINK-8212 > Project: Flink > Issue Type: Improvement > Components: Local Runtime, Network >Affects Versions: 1.5.0 >Reporter: Till Rohrmann >Priority: Major > Fix For: 1.5.0 > > > We should pull the {{EnvironmentInformation}} out of the > {{TaskManagerServices}} where it is used to get access to the memory settings > of the executing JVM. This unnecessarily couples the former with the latter > and makes testing extremely hard (one has to use {{PowerMockRunner}} and mock > the static {{EnvironmentInformation}}). > When addressing this issue, then we should also refactor > {{NetworkBufferCalculationTest}}. -- This message was sent by Atlassian JIRA (v7.6.3#76005)