You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/01/22 06:39:22 UTC
[GitHub] [flink] Myasuka opened a new pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Myasuka opened a new pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921
## What is the purpose of the change
Turn on `state.backend.rocksdb.memory.managed` to enable memory limit on RocksDB
## Brief change log
- Turn on `state.backend.rocksdb.memory.managed` to enable memory limit
- Avoid `MemoryManagerBuilder` to reserve `HEAP` memory to align with current design that memory manager only cover `OFF-HEAP` memory (see [FLINK-15023](https://issues.apache.org/jira/browse/FLINK-15023))
## Verifying this change
This change is already covered by existing tests
## 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: 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 applicable
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369718225
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
@StephanEwen I think @xintongsong might give some explanations here.
If the configured off-heap memory would not hurt, my approach works.
If the configured off-heap has a bit impact, my approach should still be picked to let flowing-up commits to fix.
In a nut shell, I'll update this with my approach before I go to sleep tonight.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369585131
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
I don't really like that we introduce this method for all sub classes of this class. Instead, it should only be relevant for RocksDB related tests. One alternative would be to introcude
```
protected void modifyMockEnvironmentBuilder(MockEnvironmentBuilder builder) {}
```
which we call in `before()` and which can be overriden by the RocksDB tests to set the managed memory size. That way, all non RocksDB related test classes would not need to implement this method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145557023 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145611482 TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4564 TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145557023) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551)
* a6df9fdaec1dff26480bfc1346d30fb718b06b5e Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145611482) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4564)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369686491
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Previously, memory manager for test would reserve on-heap memory by default. However, after [FLINK-15023](https://issues.apache.org/jira/browse/FLINK-15023), on-heap memory is not managed anymore.
As I talked with @xintongsong offline, the reason did not remove on-heap memory from memory manager for testing is mainly due to the consideration for tests could be finished more quickly if we only allocate on-heap memory for testing.
In this PR, we enable rocksDB to use managed memory by default. And I'll correct those places not setting off-heap managed memory. This might lead to some places which not need off-heap also reserved off-heap memory.
However, I found too many places to cut that redundant off-heap memory to polish the tests to the best situation and a bit out of scope of this PR targets. After I think about it for a while, I prefer to let the part to remove unnecessary off-heap memory for testing as another story or issue but not included in this PR. If so, I'll not call to set managed memory as `0` in these mem state backends.
What do you think of this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369570350
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/MockEnvironmentBuilder.java
##########
@@ -125,6 +129,13 @@ public MockEnvironmentBuilder setMetricGroup(TaskMetricGroup taskMetricGroup) {
return this;
}
+ public MockEnvironmentBuilder setIOManager(IOManager ioManager) {
+ // close previous io-manager.
+ IOUtils.closeQuietly(this.ioManager);
Review comment:
Instead of pre-initializing `ioManager` with `new IOManagerAsync()` we could leave the field `null`. In the build method we could create an `IOManagerAsync` if no other manager has been set.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369661246
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Ah I understand now. You actually want to not give managed memory to all state backend tests which don't need it. If we want to make sure that these tests actually work without `MemorySegments`, I can see the benefit of disabling managed memory for them. Maybe we could then do it the other way around meaning that we only explicitly deactivate/set managed memory to `0` for those tests but by default the `MockEnvironmentBuilder` gives you 32 MB of managed memory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #10921: [FLINK-15692][state
backend] Enable limiting RocksDB memory consumption by default
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577032625
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 2a94de63d604c3014eaaa4e0c1d6c40ff375bc4c (Wed Jan 22 06:42:13 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #10921: [FLINK-15692][state
backend] Enable limiting RocksDB memory consumption by default
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369710560
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Sounds good to me! +1 for this approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann closed pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann closed pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369582478
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/MockEnvironment.java
##########
@@ -145,8 +147,8 @@ protected MockEnvironment(
this.inputs = new LinkedList<InputGate>();
this.outputs = new LinkedList<ResultPartitionWriter>();
- this.memManager = MemoryManagerBuilder.newBuilder().setMemorySize(memorySize).build();
- this.ioManager = new IOManagerAsync();
+ this.memManager = MemoryManagerBuilder.newBuilder().setMemorySize(MemoryType.OFF_HEAP, memorySize).build();
Review comment:
```suggestion
this.memManager = MemoryManagerBuilder.newBuilder().setMemorySize(MemoryType.OFF_HEAP, offHeapMemorySize).build();
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145557023 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/145611482 TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4564 TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145557023) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551)
* a6df9fdaec1dff26480bfc1346d30fb718b06b5e Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/145611482) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4564)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/145557023 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/145557023) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369585450
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
+
@Rule
public final TemporaryFolder tempFolder = new TemporaryFolder();
+ @Before
+ public void before() {
+ env = MockEnvironment.builder().setMemorySize(getManagedMemorySize()).build();
Review comment:
I would suggest to rename `MockEnvironmentBuilder.setMemorySize` into `setManagedMemorySize`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369710339
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Not sure I fully understand.
All managed memory (on heap and off heap) is not eagerly allocated or reserved. Having some managed memory by default would not hurt, because it is not materialized as memory segments. All tests that would not use it would not behave any different if the managed memory was there or not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369580682
##########
File path: flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/tasks/StreamMockEnvironment.java
##########
@@ -158,7 +159,7 @@ public StreamMockEnvironment(
this.taskConfiguration = taskConfig;
this.inputs = new LinkedList<InputGate>();
this.outputs = new LinkedList<ResultPartitionWriter>();
- this.memManager = MemoryManagerBuilder.newBuilder().setMemorySize(memorySize).build();
+ this.memManager = MemoryManagerBuilder.newBuilder().setMemorySize(MemoryType.OFF_HEAP, memorySize).build();
Review comment:
```suggestion
this.memManager = MemoryManagerBuilder.newBuilder().setMemorySize(MemoryType.OFF_HEAP, offHeapMemorySize).build();
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] xintongsong commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
xintongsong commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369906968
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
@StephanEwen
I find it difficult to follow the entire conversation, but I can try to explain what's the problem of having on-heap managed memory for RocksDB state backend test cases:
- RocksDB state backend (or to be specific `RocksDBOperationUtils#allocateSharedCachesIfConfigured`) calls `MemoryManager#getSharedMemoryResourceForManagedMemory` for reserving and allocating managed memory, with 1.0 as the `fractionToInitializeWith`.
- In `getSharedMemoryResourceForManagedMemory`, we first compute the memory size with the fraction 1.0, then reserve that amount of memory.
- While `computeMemorySize` returns memory size including both on-heap and off-heap, we reserve only off-heap memory.
- If the memory manager contains both on-heap and off-heap memory, no matter eagerly allocated/reserved or not, we will try to reserve more off-heap memory than it has.
- E.g., memory manager contains 50mb on-heap budget and 100mb off-heap budget, in `getSharedMemoryResourceForManagedMemory` we will first compute the memory size as 150mb, then try to reserve 150mb off-heap memory which is larger than the off-heap budget.
Currently memory manager created from the testing util class `MemoryManagerBuilder` will always contain some on-heap memory, which is added in the constructor of `MemoryManagerBuilder`. When @Myasuka asked yesterday offline, I suggested to check whether any budget is set in `MemoryManagerBuilder#build`, and only add the default on-heap budget if no other budget is set. In this way, we can keep the batch test cases to use on-heap managed memory for less testing overhead, while have RocksDB state backend test cases to explicitly use off-heap managed memory only.
@tillrohrmann @Myasuka
I'm not sure whether its necessary to set managed memory to 0 for non-RocksDB cases. Since managed memory is always lazily allocated/reserved, I think streaming test cases with non-RocksDB state backend should not reserve managed memory at all, and batch test cases should only allocate on-heap managed memory if needed (which is the same behavior as before).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145557023 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145557023) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551)
* a6df9fdaec1dff26480bfc1346d30fb718b06b5e UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145557023 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145557023) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r370103547
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Thanks for the clarification. So to sum it up, it should be ok to configure off-heap managed memory for streaming test cases. Moreover, there is no harm in giving non-RocksDB test cases a bit of managed memory. If, then this should be changed follow-ups. Last but not least, batch test cases will still use on-heap managed memory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369644740
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
The original `MockEnvironmentBuilder` would already have a default `memorySize` as `1024 * 32 * 1024`. The reason I introduced a `getManagedMemorySize` method here is to let non-RocksDB test could not reserve any off-heap memory.
If you agree, I think we could use original `MockEnvironmentBuilder` directly but also reserve off-heap memory for non-RocksDB tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369585868
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendTestBase.java
##########
@@ -139,11 +142,25 @@
@Rule
public final ExpectedException expectedException = ExpectedException.none();
+ @Before
+ public void before() {
+ env = buildMockEnv();
+ }
+
+ @After
+ public void after() {
+ IOUtils.closeQuietly(env);
+ }
+
// lazily initialized stream storage
private CheckpointStreamFactory checkpointStreamFactory;
+ private MockEnvironment env;
+
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Same here with the `getManagedMemorySize`. With the above propose solution we would only need to override this method for RocksDB related classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369582314
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/query/TaskKvStateRegistry.java
##########
@@ -74,6 +75,11 @@ public void unregisterAll() {
}
}
+ @VisibleForTesting
+ public KvStateRegistry getRegistry() {
+ return registry;
+ }
Review comment:
I actually think that we should not introduce this method. Instead `MockEnvironment` could keep a reference to the created `KvStateRegistry`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369572021
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/query/TaskKvStateRegistry.java
##########
@@ -74,6 +75,11 @@ public void unregisterAll() {
}
}
+ @VisibleForTesting
+ public KvStateRegistry getRegistry() {
+ return registry;
+ }
Review comment:
Why do we need to make this method visible for testing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#issuecomment-577180069
<!--
Meta data
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145557023 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551 TriggerType:PUSH TriggerID:adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145611482 TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
Hash:a6df9fdaec1dff26480bfc1346d30fb718b06b5e Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4564 TriggerType:PUSH TriggerID:a6df9fdaec1dff26480bfc1346d30fb718b06b5e
-->
## CI report:
* adcb12e06b2b850c2e3757c9f43f3d2aaf8599fe Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145557023) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4551)
* a6df9fdaec1dff26480bfc1346d30fb718b06b5e Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145611482) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4564)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369710339
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
Not sure I fully understand.
All managed memory (on heap and off heap) is not eagerly allocated or reserved. Having some managed memory by default would not hurt, because it is not materialized as memory segments. All tests that would not use it would not behave any different if the managed memory was configured or not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] xintongsong commented on a change in pull request #10921:
[FLINK-15692][state backend] Enable limiting RocksDB memory consumption by
default
Posted by GitBox <gi...@apache.org>.
xintongsong commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369906968
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
##########
@@ -71,12 +74,26 @@
protected abstract B getStateBackend() throws Exception;
+ protected abstract long getManagedMemorySize();
Review comment:
@StephanEwen
I find it difficult to follow the entire conversation, but I can try to explain what's the problem of having on-heap managed memory for RocksDB state backend test cases:
- RocksDB state backend (or to be specific `RocksDBOperationUtils#allocateSharedCachesIfConfigured`) calls `MemoryManager#getSharedMemoryResourceForManagedMemory` for reserving and allocating managed memory, with 1.0 as the `fractionToInitializeWith`.
- In `getSharedMemoryResourceForManagedMemory`, we first compute the memory size with the fraction 1.0, then reserve that amount of memory.
- While `computeMemorySize` returns memory size including both on-heap and off-heap, we reserve only off-heap memory.
- If the memory manager contains both on-heap and off-heap memory, no matter eagerly allocated/reserved or not, we will try to reserve more off-heap memory than it has.
- E.g., memory manager contains 50mb on-heap budget and 100mb off-heap budget, in `getSharedMemoryResourceForManagedMemory` we will first compute the memory size as 150mb, then try to reserve 150mb off-heap memory which is larger than the off-heap budget.
Currently memory manager created from the testing util class `MemoryManagerBuilder` will always contain some on-heap memory, which is added in the constructor of `MemoryManagerBuilder`. When @Myasuka asked yesterday offline, I suggested to check whether any budget is set in `MemoryManagerBuilder#build`, and only add the default on-heap budget if no other budget is set. In this way, we can keep the batch test cases to use on-heap managed memory for less testing overhead, while have RocksDB state backend test cases to explicitly use off-heap managed memory only.
An alternative could be make `MemoryManager#computeMemorySize` returns only size of off-heap memory. However, since we do not have on-heap managed memory in production, I'm in favor of keep the concern of excluding on-heap memory in testing codes only.
@tillrohrmann @Myasuka
I'm not sure whether its necessary to set managed memory to 0 for non-RocksDB cases. Since managed memory is always lazily allocated/reserved, I think streaming test cases with non-RocksDB state backend should not reserve managed memory at all, and batch test cases should only allocate on-heap managed memory if needed (which is the same behavior as before).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services