anonymouscodeholic commented on pull request #13393: URL: https://github.com/apache/flink/pull/13393#issuecomment-712108887
> Logs a WARN if the case arena block size is too high. > > ## What is the purpose of the change > This change adds a WARN message in the case RocksDB will very likely (100% sure?) perform very poorly because of the way the managed memory is configured. > > There's an associated JIRA ticket [FLINK-19238](https://issues.apache.org/jira/browse/FLINK-19238). > > ## Brief change log > `RocksDBMemoryControllerUtils` has new methods to calculate different values as RocksDB calculates those. > > The actual logging is in `RocksDBOperationUtils.sanityCheckArenaBlockSize`. The arena block size is not known before this. Most of the code changes is caused by the need to pass `writeBufferManagerCapacity` to the check. > > ## Verifying this change > If you run any properly performing Flink application (on a RocksDB state backend), there won't be any change caused by this PR. > > If you setup a simple Flink app and run it with `StreamExecutionEnvironment.createLocalEnvironment` and high parallelism (let's say 16), you should see the new WARN messages > > This change added tests and can be verified as follows: > > * `RocksDBOperationsUtilsTest.testSanityCheckArenaBlockSize` > > ## 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, Kubernetes/Yarn/Mesos, ZooKeeper: no > * The S3 file system connector: no > > ## Documentation > * Does this pull request introduce a new feature? yes/no. The only user visible change is a new WARN message. Not really a feature... > * If yes, how is the feature documented? JavaDocs > Logs a WARN if the case arena block size is too high. > > ## What is the purpose of the change > This change adds a WARN message in the case RocksDB will very likely (100% sure?) perform very poorly because of the way the managed memory is configured. > > There's an associated JIRA ticket [FLINK-19238](https://issues.apache.org/jira/browse/FLINK-19238). > > ## Brief change log > `RocksDBMemoryControllerUtils` has new methods to calculate different values as RocksDB calculates those. > > The actual logging is in `RocksDBOperationUtils.sanityCheckArenaBlockSize`. The arena block size is not known before this. Most of the code changes is caused by the need to pass `writeBufferManagerCapacity` to the check. > > ## Verifying this change > If you run any properly performing Flink application (on a RocksDB state backend), there won't be any change caused by this PR. > > If you setup a simple Flink app and run it with `StreamExecutionEnvironment.createLocalEnvironment` and high parallelism (let's say 16), you should see the new WARN messages > > This change added tests and can be verified as follows: > > * `RocksDBOperationsUtilsTest.testSanityCheckArenaBlockSize` > > ## 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, Kubernetes/Yarn/Mesos, ZooKeeper: no > * The S3 file system connector: no > > ## Documentation > * Does this pull request introduce a new feature? yes/no. The only user visible change is a new WARN message. Not really a feature... > * If yes, how is the feature documented? JavaDocs > > > Hey. I'm going to lose access to the GitHub account soon, and not be able to modify the branch anymore. If changes are needed before merge, I guess I could create a new PR using another GH account. > > > > > > @juha-mynttinen-king sorry for the late reply due to the National Day Holiday here, will check the updated changes asap. > > About the GH account, yes it's ok to create another PR and link it with this one, while I suggest to use the same account as long as possible in the future (if it's impossible with the existing one) so it could better reflect your coding career (smile). > > I (previously @juha-mynttinen-king) switched over to this account. I think we can keep this PR, if that's fine for you @carp84. Actually, it didn't work out. I couldn't push to the original branch anymore. I created a new PR https://github.com/apache/flink/pull/13688 and fixed things according your comments @carp84 there. This PR can be closed. ---------------------------------------------------------------- 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: [email protected]
