feat(config): remove ineffective RocksDB table setters - #50
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bd3ec32 to
50ceeba
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="common/src/main/java/org/tron/common/setting/RocksDbSettings.java">
<violation number="1" location="common/src/main/java/org/tron/common/setting/RocksDbSettings.java:204">
P3: This change removed the only usage of `RocksDbSettings.getCache()`, so the static `cache` field now allocates a 1 GB native LRUCache on class load that nothing consumes. Remove the `cache` field and the `getCache()` method, or the allocation is wasted native memory on every process.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/config/ConfigurationTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/config/ConfigurationTest.java:101">
P3: assertFalse(config.hasPath("storage.dbSettings.blocksize")) can never detect a regression because that key does not exist in config.conf, reference.conf, or DbSettingsConfig. The per-database blockSize override lives under storage.properties (DbOptionOverride.blockSize), not storage.dbSettings. Drop the assertion, or if you intend to guard the removal, assert on the namespace that actually carried the option.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| tableCfg.setCacheIndexAndFilterBlocks(true); | ||
| tableCfg.setPinL0FilterAndIndexBlocksInCache(true); | ||
| tableCfg.setFilter(new BloomFilter(10, false)); | ||
| options.setTableFormatConfig(new BlockBasedTableConfig()); |
There was a problem hiding this comment.
P3: This change removed the only usage of RocksDbSettings.getCache(), so the static cache field now allocates a 1 GB native LRUCache on class load that nothing consumes. Remove the cache field and the getCache() method, or the allocation is wasted native memory on every process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/main/java/org/tron/common/setting/RocksDbSettings.java, line 204:
<comment>This change removed the only usage of `RocksDbSettings.getCache()`, so the static `cache` field now allocates a 1 GB native LRUCache on class load that nothing consumes. Remove the `cache` field and the `getCache()` method, or the allocation is wasted native memory on every process.</comment>
<file context>
@@ -211,13 +201,7 @@ protected void log(InfoLogLevel infoLogLevel, String logMsg) {
- tableCfg.setCacheIndexAndFilterBlocks(true);
- tableCfg.setPinL0FilterAndIndexBlocksInCache(true);
- tableCfg.setFilter(new BloomFilter(10, false));
+ options.setTableFormatConfig(new BlockBasedTableConfig());
if (Constant.MARKET_PAIR_PRICE_TO_ORDER.equals(dbName)) {
ComparatorOptions comparatorOptions = new ComparatorOptions();
</file context>
There was a problem hiding this comment.
The 1 GiB value is the cache capacity limit and is not eagerly allocated as 1 GiB of physical memory. Since the cache is no longer attached to the native table factory, only the cache object and its metadata remain allocated. We will retain the existing field and accessor for compatibility and handle its removal, sizing, and lifecycle separately after validating an appropriate cache configuration.
| Config config = Configuration.getByFileName("config.conf"); | ||
| StorageConfig.DbSettingsConfig settings = StorageConfig.fromConfig(config).getDbSettings(); | ||
|
|
||
| assertFalse(config.hasPath("storage.dbSettings.blocksize")); |
There was a problem hiding this comment.
P3: assertFalse(config.hasPath("storage.dbSettings.blocksize")) can never detect a regression because that key does not exist in config.conf, reference.conf, or DbSettingsConfig. The per-database blockSize override lives under storage.properties (DbOptionOverride.blockSize), not storage.dbSettings. Drop the assertion, or if you intend to guard the removal, assert on the namespace that actually carried the option.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/config/ConfigurationTest.java, line 101:
<comment>assertFalse(config.hasPath("storage.dbSettings.blocksize")) can never detect a regression because that key does not exist in config.conf, reference.conf, or DbSettingsConfig. The per-database blockSize override lives under storage.properties (DbOptionOverride.blockSize), not storage.dbSettings. Drop the assertion, or if you intend to guard the removal, assert on the namespace that actually carried the option.</comment>
<file context>
@@ -91,4 +92,19 @@ public void getConfigurationWhenOnlyConfFileName() {
+ Config config = Configuration.getByFileName("config.conf");
+ StorageConfig.DbSettingsConfig settings = StorageConfig.fromConfig(config).getDbSettings();
+
+ assertFalse(config.hasPath("storage.dbSettings.blocksize"));
+ assertEquals(7, settings.getLevelNumber());
+ assertEquals(256, settings.getMaxBytesForLevelBase());
</file context>
| assertFalse(config.hasPath("storage.dbSettings.blocksize")); | |
| // (remove the assertFalse on storage.dbSettings.blocksize; the key was never defined) |
There was a problem hiding this comment.
Addressed in a3bab81b05. The storage.dbSettings.blocksize entry is now retained in both config.conf and reference.conf to avoid an unnecessary configuration compatibility change. The configuration test verifies that the shipped value still resolves to 64 KiB, while the native Options test separately verifies that it is not applied to the RocksDB table factory.
9ecc46a to
2594672
Compare
Preserve the shipped RocksDB configuration and public block-size API while removing table setters that never reached the native table factory. Document that blocksize is currently ineffective and verify native behavior. Keep the existing shared cache object for separate configuration design and validation. Closes #49
2594672 to
a3bab81
Compare
| dbSettings = { | ||
| levelNumber = 7 // Number of RocksDB levels. | ||
| compactThreads = 0 // 0 = auto: max(availableProcessors, 1) | ||
| # Currently retained for compatibility but not applied to native RocksDB table options. |
There was a problem hiding this comment.
[SHOULD] Align the PR metadata with the retained configuration
The PR title and authored What/Why sections still say that the shipped config.conf overrides are removed so reference.conf defaults take effect. At this HEAD, config.conf and the block-size API remain unchanged; the implementation only removes table setters that never reached the native table factory and adds this compatibility note. The current text therefore describes a configuration/default change that does not occur, and its test list also omits RocksDbSettingsTest.
Suggestion: Rename the PR and update What/Why/Tests to describe the retained configuration and API, the removed inactive setters, and the native OPTIONS-* regression test.
bladehan1
changed the title
1 participant
What does this PR do?
Remove RocksDB block-table setters that were applied after
setTableFormatConfig()and therefore never reached the native table factory.Keep the existing
storage.dbSettings.blocksizeentries and Java API for compatibility, while documenting inreference.confthat the value is not currently applied to native RocksDB table options.Add regression tests for the shipped Options-level configuration and the effective native block-table behavior.
Why are these changes required?
setTableFormatConfig()materializes the native table factory from the Java configuration at call time. The later block-size, shared-cache, index/filter-cache, L0-pinning, and Bloom-filter setters only changed the Java object and did not affect the native factory.Moving those setters before
setTableFormatConfig()would activate several unverified settings at once. In a fixed 500-block Mainnet replay, activating the combined settings reduced throughput from 2.421 blocks/s to 1.969 and 1.680 blocks/s, while restoring the pre-change effective native behavior produced 2.418 blocks/s.This change therefore removes the ineffective setter sequence while preserving the behavior that nodes actually used before the change.
This PR has been tested by:
./gradlew :common:test --tests org.tron.common.setting.RocksDbSettingsTest./gradlew :framework:test --tests org.tron.core.config.ConfigurationTest./gradlew :framework:checkstyleMain./gradlew :framework:checkstyleTestFollow up
Evaluate block size, Bloom filters, shared-cache capacity, ownership, lifecycle, and observability independently with representative SR and database-specific workloads before enabling another RocksDB table profile.
Extra details
blocksizeremains configurable and observable for compatibility but is not applied to the native table factory.Closes #49
Summary by cubic
Uses RocksDB’s native block-table defaults and keeps
storage.dbSettings.blocksizeonly for compatibility. Old behavior set block size, bloom filter, and cached index/filter blocks; new behavior sets no table options (defaults: 4 KB blocks, no bloom, index/filter not cached). No API, protocol, or DB-format changes.Refactors
reference.confthatblocksizeis not applied to table options.Written for commit a3bab81. Summary will update on new commits.