[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 cc @dawidwys ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 cc @dawidwys introduced `MemoryUnit` and refactor `MemorySize`, please review~ ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 cc @dawidwys the third suggestion has finished, the others has supported before. can you review this? ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5448 Hi @yanghua, I am afraid Stephan won't be able to reply any time soon. I would suggest to - add the default unit to the parse method of `MemorySize` and use MB for `MANAGED_MEMORY_SIZE`. - change the return value of getMebiBytes() to int or have a getMebiBytesAsInt() method that uses a MathUtils.checkedDownCast() to avoid unnoticed overflow errors, as Stephan commented - change the default value of `MANAGED_MEMORY_SIZE` to 0, as suggested by @zentol After that I think this PR will be ready to be merged. ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 @StephanEwen any opinion about this PR? ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5448 There is no problem reusing old keys, if their default unit was "bytes", because the `MemorySize.parse(...)` interprets a number as bytes, if there is no unit attached to it. I did not realize that you switched the config keys already - in that case we need to backwards support the old keys as well. Also, we need to update all the shell scripts (`config.sh`, `jobmanager.sh`, `taskmanager.sh` and so on) to be consistent with the new config keys. ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 @StephanEwen for the `taskmanager.memory.segment-size` because of it's default unit is `byte`, so whether there is a unit or not, the behavior is consistent. So we just need to handle `taskmanager.memory.size` ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 @StephanEwen for the open question, this PR's implementation has used some new key and deprecated the old key, such as `jobmanager.heap.mb -> jobmanager.heap.size` , `taskmanager.heap.mb -> taskmanager.heap.size`, the problem is how to deal with `taskmanager.memory.segment-size` and `taskmanager.memory.size` which contains the keyword `size` is not suitable to be replaced by a new key. ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5448 Okay, after taking a look, I think we need to add a few changes: - We need to add an additional `MemoryUnit.parse()` method that takes the "default" unit, so that we parse the old heap sizes such that they are in MB if nothing else is specified. - We should either change the return value of `getMebiBytes()` to `int` or have a `getMebiBytesAsInt()` method that uses a `MathUtils.checkedDownCast()` to avoid unnoticed overflow errors. Open question: As we are changing the value type of the heap size config options, should we deprecate the current config keys and introduce new ones (like `jobmanager.heap-size`)? ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5448 Will try and take a look at this soon... Sorry for the delay. What I would consider very important is that users who don't change their configuration do not get different behavior all of a sudden. Meaning in the absence of a "unit" we do not always interpret the value as a "byte" but as whatever the config value was measured in before (such as MBs, ...). ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 hi @dawidwys thanks for your review suggestion, I have refactored the PR code except the `MANAGED_MEMORY_SIZE `. The problem you concerned is exists, the key is suitable for this PR, and it seems we should introduce a new key and mark this as **@deprecated**, otherwise, We could not avoid user using old config value without unit. But it is just a possibility, we can highlight a **Note** message and give a guidance. At worst, user config it as **megabytes**, actually it means **bytes"" (`1024 * 1024` difference) , when starting TaskManager, it would cause failed or show disagreement with Flink web UI. Then use would recheck this configuration item. What's your opinion? @StephanEwen ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5448 Hi @yanghua I think those changes look overall good. The biggest concern I had with handling `MANAGED_MEMORY_SIZE` as I think if used with old style configuration file, the resulting value will differ. ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 cc @dawidwys and @StephanEwen , I have fixed the conflicts and some issues in new files, please review it. ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5448 @yanghua Could you rebase this PR on top of the master? I could have a look then. Thanks! ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5448 Thanks, I think this is a really nice change! Given that we are approaching feature freeze for 1.5 and already have a very big backlog, I would try and get to this for the 1.6 release. Hope that works for you. ---
[GitHub] flink issue #5448: [FLINK-6469] Configure Memory Sizes with units
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5448 Hi @StephanEwen , with the help of your `MemorySize` (sub task of FLINK-6469)ï¼I finished the remained work, replaced the old memory config (in codeãconfig file and shell script) with the memory size. For the config item in `conf/flink-conf.yaml`, retained but deprecated the config key `jobmanager.heap.mb` , `taskmanager.heap.mb` and introduced `jobmanager.heap.size`, `taskmanager.heap.size` with the unit `m`. Would you please review this PR, thanks! ---
