[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958505#comment-15958505 ] ASF GitHub Bot commented on FLINK-6270: --- GitHub user NicoK opened a pull request: https://github.com/apache/flink/pull/3683 [FLINK-6270] Port several network config parameters to ConfigOption This ports some memory and network buffers related config options to new `ConfigOption` instances. These include: * `taskmanager.memory.size` * `taskmanager.memory.fraction` * `taskmanager.memory.off-heap` * `taskmanager.memory.preallocate` * `taskmanager.network.numberOfBuffers` * `taskmanager.memory.segment-size` Some of these already existed as `ConfigOption` instances in `MiniClusterConfiguration`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/NicoK/flink flink-6270 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3683.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 #3683 commit 15ae80ee5a9cd29597f6ed597183251fbbb32f37 Author: Nico Kruber Date: 2017-04-05T08:59:00Z [FLINK-6270] extend Configuration with contains(configOption) commit f136af323bce2517983af6c47849c9c70dc3efc2 Author: Nico Kruber Date: 2017-04-05T09:45:57Z [FLINK-6270] port some memory and network task manager options to ConfigOption > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958535#comment-15958535 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110102292 --- Diff: flink-runtime/src/main/scala/org/apache/flink/runtime/minicluster/LocalFlinkMiniCluster.scala --- @@ -350,23 +350,18 @@ class LocalFlinkMiniCluster( def setMemory(config: Configuration): Unit = { // set this only if no memory was pre-configured -if (config.getInteger(ConfigConstants.TASK_MANAGER_MEMORY_SIZE_KEY, -1) == -1) { +if (config.getLong(TaskManagerOptions.MANAGED_MEMORY_SIZE) == -1L) { --- End diff -- If we keep the default at -1 then we should compare against `TaskManagerOptons.MANAGED_MEMORY_SIZE.defaultValue()`; the same applies to other instances of this pattern. > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958536#comment-15958536 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110104138 --- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java --- @@ -595,6 +595,35 @@ public boolean containsKey(String key){ } } + /** +* Checks whether there is an entry for the given config option +* +* @param configOption The configuration option +* +* @return true if a valid (current of deprecated) key of the config option is stored, +* false otherwise +*/ + public boolean contains(ConfigOption configOption) { --- End diff -- Could you annotate this with `@PublicEvolving` similar to other methods using `ConfigOption`s. > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958537#comment-15958537 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110103104 --- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java --- @@ -595,6 +595,35 @@ public boolean containsKey(String key){ } } + /** +* Checks whether there is an entry for the given config option +* +* @param configOption The configuration option +* +* @return true if a valid (current of deprecated) key of the config option is stored, --- End diff -- typo: current or deprecated > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958534#comment-15958534 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110102530 --- Diff: flink-core/src/main/java/org/apache/flink/configuration/TaskManagerOptions.java --- @@ -39,10 +39,53 @@ key("taskmanager.jvm-exit-on-oom") .defaultValue(false); + /** Size of memory buffers used by the network stack and the memory manager (in bytes). */ + public static final ConfigOption MEMORY_SEGMENT_SIZE = + key("taskmanager.memory.segment-size") + .defaultValue(32768); + + /** +* Amount of memory to be allocated by the task manager's memory manager (in megabytes). If not +* set, a relative fraction will be allocated, as defined by {@link #MANAGED_MEMORY_FRACTION}. +*/ + public static final ConfigOption MANAGED_MEMORY_SIZE = + key("taskmanager.memory.size") + .defaultValue(-1L); --- End diff -- This seems like an odd default value since it isn't actually a valid value for it. All usages where the value is -1 have a special case for it; `.noDefaultValue()` seems more appropriate. > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958555#comment-15958555 ] ASF GitHub Bot commented on FLINK-6270: --- Github user NicoK commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110106044 --- Diff: flink-core/src/main/java/org/apache/flink/configuration/TaskManagerOptions.java --- @@ -39,10 +39,53 @@ key("taskmanager.jvm-exit-on-oom") .defaultValue(false); + /** Size of memory buffers used by the network stack and the memory manager (in bytes). */ + public static final ConfigOption MEMORY_SEGMENT_SIZE = + key("taskmanager.memory.segment-size") + .defaultValue(32768); + + /** +* Amount of memory to be allocated by the task manager's memory manager (in megabytes). If not +* set, a relative fraction will be allocated, as defined by {@link #MANAGED_MEMORY_FRACTION}. +*/ + public static final ConfigOption MANAGED_MEMORY_SIZE = + key("taskmanager.memory.size") + .defaultValue(-1L); --- End diff -- yes, but unfortunately, `.noDefaultValue` is only specified for `ConfigOption` - I'll change the comparisons to match against the default though - that already looked like a strange pattern > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958572#comment-15958572 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110107855 --- Diff: flink-core/src/main/java/org/apache/flink/configuration/TaskManagerOptions.java --- @@ -39,10 +39,53 @@ key("taskmanager.jvm-exit-on-oom") .defaultValue(false); + /** Size of memory buffers used by the network stack and the memory manager (in bytes). */ + public static final ConfigOption MEMORY_SEGMENT_SIZE = + key("taskmanager.memory.segment-size") + .defaultValue(32768); + + /** +* Amount of memory to be allocated by the task manager's memory manager (in megabytes). If not +* set, a relative fraction will be allocated, as defined by {@link #MANAGED_MEMORY_FRACTION}. +*/ + public static final ConfigOption MANAGED_MEMORY_SIZE = + key("taskmanager.memory.size") + .defaultValue(-1L); --- End diff -- Ok, didn't know that only works for strings:/ > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958575#comment-15958575 ] ASF GitHub Bot commented on FLINK-6270: --- Github user NicoK commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110108276 --- Diff: flink-runtime/src/main/scala/org/apache/flink/runtime/minicluster/LocalFlinkMiniCluster.scala --- @@ -350,23 +350,18 @@ class LocalFlinkMiniCluster( def setMemory(config: Configuration): Unit = { // set this only if no memory was pre-configured -if (config.getInteger(ConfigConstants.TASK_MANAGER_MEMORY_SIZE_KEY, -1) == -1) { +if (config.getLong(TaskManagerOptions.MANAGED_MEMORY_SIZE) == -1L) { --- End diff -- I'm tempted to replace these with a `config.contains()` call but then the user would not be able to explicitly provide `-1` as the config option's value which is possible and valid at the moment. I don't really want to break existing behaviour though. What do you think? > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958591#comment-15958591 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3683#discussion_r110110002 --- Diff: flink-runtime/src/main/scala/org/apache/flink/runtime/minicluster/LocalFlinkMiniCluster.scala --- @@ -350,23 +350,18 @@ class LocalFlinkMiniCluster( def setMemory(config: Configuration): Unit = { // set this only if no memory was pre-configured -if (config.getInteger(ConfigConstants.TASK_MANAGER_MEMORY_SIZE_KEY, -1) == -1) { +if (config.getLong(TaskManagerOptions.MANAGED_MEMORY_SIZE) == -1L) { --- End diff -- i think we'll have to stick to comparisons for this one :( > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15958927#comment-15958927 ] ASF GitHub Bot commented on FLINK-6270: --- Github user zentol commented on the issue: https://github.com/apache/flink/pull/3683 merging. > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (FLINK-6270) Port several network config parameters to ConfigOption
[ https://issues.apache.org/jira/browse/FLINK-6270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15959397#comment-15959397 ] ASF GitHub Bot commented on FLINK-6270: --- Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3683 > Port several network config parameters to ConfigOption > -- > > Key: FLINK-6270 > URL: https://issues.apache.org/jira/browse/FLINK-6270 > Project: Flink > Issue Type: Improvement > Components: Network >Affects Versions: 1.3.0 >Reporter: Nico Kruber >Assignee: Nico Kruber >Priority: Minor > > I'd like to port some memory and network buffers related config options to > new {{ConfigOption}} instances before continuing with FLINK-4545. These > include: > * {{taskmanager.memory.size}} > * {{taskmanager.memory.fraction}} > * {{taskmanager.memory.off-heap}} > * {{taskmanager.memory.preallocate}} > * {{taskmanager.network.numberOfBuffers}} > * {{taskmanager.memory.segment-size}} > Some of these already existed as {{ConfigOption}} instances in > {{MiniClusterConfiguration}}. -- This message was sent by Atlassian JIRA (v6.3.15#6346)