Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5448
cc @dawidwys
---
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5448
cc @dawidwys introduced `MemoryUnit` and refactor `MemorySize`, please
review~
---
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 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
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5448
@StephanEwen any opinion about this PR?
---
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 r
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 `t
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 -> taskmanag
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
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
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
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
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 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 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. Ho
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 s
16 matches
Mail list logo
