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 2023/01/19 07:18:28 UTC
[GitHub] [flink] dchristle opened a new pull request, #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
dchristle opened a new pull request, #21729:
URL: https://github.com/apache/flink/pull/21729
<!--
*Thank you very much for contributing to Apache Flink - we are happy that you want to help us improve Flink. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
*Please understand that we do not do this to make contributions to Flink a hassle. In order to uphold a high standard of quality for code contributions, while at the same time managing a large number of contributions, we need contributors to prepare the contributions well, and give reviewers enough contextual information for the review. Please also understand that contributions that do not follow this guide will take longer to review and thus typically be picked up with lower priority by the community.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [JIRA issue](https://issues.apache.org/jira/projects/FLINK/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no JIRA issue.
- Name the pull request in the form "[FLINK-XXXX] [component] Title of the pull request", where *FLINK-XXXX* should be replaced by the actual issue number. Skip *component* if you are unsure about which is the best component.
Typo fixes that have no associated JIRA issue should be named following this pattern: `[hotfix] [docs] Fix typo in event time introduction` or `[hotfix] [javadocs] Expand JavaDoc for PuncuatedWatermarkGenerator`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes. You can set up Azure Pipelines CI to do that following [this guide](https://cwiki.apache.org/confluence/display/FLINK/Azure+Pipelines#AzurePipelines-Tutorial:SettingupAzurePipelinesforaforkoftheFlinkrepository).
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message (including the JIRA id)
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
## What is the purpose of the change
This PR corrects and improves the readability of the documentation for RocksDB predefined option sets. References to `disableDataSync`, which is not set and has been removed from RocksDB for some time, are removed. I also sorted the options lexicographically, and did the same for the code that sets these options. This makes it easier to read & spot bugs. While doing this, I noticed the `SPINNING_DISK_OPTIMIZED_HIGH_MEM` predefined option set actually enables Bloom filter usage, but it isn't documented to do that. I added brief documentation for it. There are some other minor inconsistencies in the Python vs Java documentation that I tried to match up, too.
There are also some references to Flink not needing to sync changes to the file system due to its checkpointing ability. `setUseFsync` is set to `false`, it appears, for all settings. Currently, there are some references to not needing syncing in the Java documentation, but they don't quite make sense because `setUseFsync` is not mentioned there. Instead, as the existing documentation suggests, options common to each predefined set are set elsewhere. So, I removed most duplicate references to `setUseFsync`.
## Brief change log
* Remove references to `disableDataSync` in RocksDB documentation. This is not set, and has been removed from RocksDB for some time.
* Make Python documentation consistent with Java documentation by removing `setUseFsync` references on each option.
* Sort options in both Java and Python documentation.
* Sort the code setting configuration to match the sorted-order documentation.
* Change use of `setIncreaseParallelism` in Python documentation to `setMaxBackgroundJobs`, which matches the Java usage.
* Add missing Bloom filter option to the `SPINNING_DISK_OPTIMIZED_HIGH_MEM` documentation, and describe where its parameters are set.
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## 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, 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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dchristle commented on pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by "dchristle (via GitHub)" <gi...@apache.org>.
dchristle commented on PR #21729:
URL: https://github.com/apache/flink/pull/21729#issuecomment-1575702300
@Myasuka Got it - I just rebased on the latest master branch.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on a diff in pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #21729:
URL: https://github.com/apache/flink/pull/21729#discussion_r1080950829
##########
flink-python/pyflink/datastream/state_backend.py:
##########
@@ -956,16 +956,20 @@ class PredefinedOptions(Enum):
determined to be beneficial for performance under different settings.
Some of these settings are based on experiments by the Flink community, some follow
- guides from the RocksDB project.
+ guides from the RocksDB project. Some configurations are enabled unconditionally (e.g.
+ setUseFsync(false), which disables syncing to storage) so they do not appear here. See the
+ documentation for the createBaseCommonDBOptions() and createBaseCommonColumnOptions() methods
Review Comment:
`createBaseCommonDBOptions` and `createBaseCommonColumnOptions` is not public in `RocksDBResourceContainer`. I think we don't need to emphasize this.
##########
flink-python/pyflink/datastream/state_backend.py:
##########
@@ -1000,21 +1000,24 @@ class PredefinedOptions(Enum):
The following options are set:
+ - BlockBasedTableConfig.setBlockCacheSize(256 MBytes)
+ - BlockBasedTableConfig.setBlockSize(128 KBytes)
+ - BlockBasedTableConfig.setFilterPolicy(BloomFilter(
+ `BLOOM_FILTER_BITS_PER_KEY`,
+ `BLOOM_FILTER_BLOCK_BASED_MODE`)
- setLevelCompactionDynamicLevelBytes(true)
- - setTargetFileSizeBase(256 MBytes)
+ - setMaxBackgroundJobs(4)
- setMaxBytesForLevelBase(1 GByte)
- - setWriteBufferSize(64 MBytes)
- - setIncreaseParallelism(4)
- - setMinWriteBufferNumberToMerge(3)
- - setMaxWriteBufferNumber(4)
- - setUseFsync(false)
- setMaxOpenFiles(-1)
- - BlockBasedTableConfig.setBlockCacheSize(256 MBytes)
- - BlockBasedTableConfigsetBlockSize(128 KBytes)
+ - setMaxWriteBufferNumber(4)
Review Comment:
Why we need to change the orders of these settings in this PR?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21729:
URL: https://github.com/apache/flink/pull/21729#issuecomment-1396544048
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3a904a93192be9bf612e76edbfa39465b398404",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "c3a904a93192be9bf612e76edbfa39465b398404",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c3a904a93192be9bf612e76edbfa39465b398404 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka commented on pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by "Myasuka (via GitHub)" <gi...@apache.org>.
Myasuka commented on PR #21729:
URL: https://github.com/apache/flink/pull/21729#issuecomment-1569447822
@dchristle Thanks for the reminder, I will take a look recently.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dchristle commented on pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by "dchristle (via GitHub)" <gi...@apache.org>.
dchristle commented on PR #21729:
URL: https://github.com/apache/flink/pull/21729#issuecomment-1576170197
@Myasuka Thanks for your review & help here!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dchristle commented on a diff in pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by GitBox <gi...@apache.org>.
dchristle commented on code in PR #21729:
URL: https://github.com/apache/flink/pull/21729#discussion_r1081619201
##########
flink-python/pyflink/datastream/state_backend.py:
##########
@@ -956,16 +956,20 @@ class PredefinedOptions(Enum):
determined to be beneficial for performance under different settings.
Some of these settings are based on experiments by the Flink community, some follow
- guides from the RocksDB project.
+ guides from the RocksDB project. Some configurations are enabled unconditionally (e.g.
+ setUseFsync(false), which disables syncing to storage) so they do not appear here. See the
+ documentation for the createBaseCommonDBOptions() and createBaseCommonColumnOptions() methods
Review Comment:
That makes sense. I originally made this change to make PyFlink documentation match closely to the same description from the Java documentation, but mentioning the Java classes & non-public methods probably adds confusion. I made this change in the most recent commits.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dchristle commented on pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by "dchristle (via GitHub)" <gi...@apache.org>.
dchristle commented on PR #21729:
URL: https://github.com/apache/flink/pull/21729#issuecomment-1569381024
Hi @Myasuka Is there any chance we can merge this PR?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dchristle commented on a diff in pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by GitBox <gi...@apache.org>.
dchristle commented on code in PR #21729:
URL: https://github.com/apache/flink/pull/21729#discussion_r1081578012
##########
flink-python/pyflink/datastream/state_backend.py:
##########
@@ -1000,21 +1000,24 @@ class PredefinedOptions(Enum):
The following options are set:
+ - BlockBasedTableConfig.setBlockCacheSize(256 MBytes)
+ - BlockBasedTableConfig.setBlockSize(128 KBytes)
+ - BlockBasedTableConfig.setFilterPolicy(BloomFilter(
+ `BLOOM_FILTER_BITS_PER_KEY`,
+ `BLOOM_FILTER_BLOCK_BASED_MODE`)
- setLevelCompactionDynamicLevelBytes(true)
- - setTargetFileSizeBase(256 MBytes)
+ - setMaxBackgroundJobs(4)
- setMaxBytesForLevelBase(1 GByte)
- - setWriteBufferSize(64 MBytes)
- - setIncreaseParallelism(4)
- - setMinWriteBufferNumberToMerge(3)
- - setMaxWriteBufferNumber(4)
- - setUseFsync(false)
- setMaxOpenFiles(-1)
- - BlockBasedTableConfig.setBlockCacheSize(256 MBytes)
- - BlockBasedTableConfigsetBlockSize(128 KBytes)
+ - setMaxWriteBufferNumber(4)
Review Comment:
Originally, I wanted to remove just the erroneous `disableDataSync`, but in doing this, I noticed other errors like the missing Bloom filter or that the Python documentation implies `setUseFSync(false)` is set while the Java docs don't (not technically a mistake, but it's confusing & not easy to see from the code).
Sorting isn't strictly necessary, but it _was_ part of how I found the Bloom filter setting was undocumented & manually verified each group of options now matches its description. It lets readers quickly determine whether a particular setting they have in mind is changed or not. Since it is easier to see at a glance when a new config is added, or an old one removed, future maintainers have a lower probability of introducing bugs or making the documentation & code diverge like it did for the Bloom filter.
It isn't strictly necessary to sort in this PR, either, but the overhead of creating a separate PR/separate JIRA issue seems too high for such a simple change.
I can add these other non-idealities that I fixed into the JIRA issue description. Would that be an OK path forward, rather than deferring these minor polishes into separate PRs?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] Myasuka closed pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by "Myasuka (via GitHub)" <gi...@apache.org>.
Myasuka closed pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
URL: https://github.com/apache/flink/pull/21729
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] dchristle commented on pull request #21729: [FLINK-30751] [docs] Remove references to disableDataSync in RocksDB documentation
Posted by GitBox <gi...@apache.org>.
dchristle commented on PR #21729:
URL: https://github.com/apache/flink/pull/21729#issuecomment-1397381115
Hi @Myasuka,
Thanks for reviewing this PR! I split the changes into separate commits. The first commit removes `disableDataSync`, and the last commit aligns the PyFlink RocksDB documentation with its Java equivalent, as you requested.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org