Avoid duplicate DataNode memory configuration initialization - #18467
Avoid duplicate DataNode memory configuration initialization#18467jt2594838 wants to merge 2 commits into
Conversation
|
|
||
| protected IoTDBDescriptor() { | ||
| loadProps(); | ||
| boolean hasLoadedProperties = loadProps(); |
There was a problem hiding this comment.
Capture whether the system properties source was loaded because loadProperties initializes memoryConfig as part of that path. The constructor needs this state to distinguish an already configured memory manager from the no-configuration fallback.
| /** load a property file and set TsfileDBConfig variables. */ | ||
| @SuppressWarnings("squid:S3776") // Suppress high Cognitive Complexity warning | ||
| private void loadProps() { | ||
| private boolean loadProps() { |
There was a problem hiding this comment.
Return a boolean from loadProps so callers can tell whether this method reached the configuration-loading path. The true and false returns mirror the existing URL-present and URL-absent branches without changing their error handling.
| // if there are no properties, we need to init memory config | ||
| if (!hasProperties) { | ||
| // If no configuration source initialized the memory config, initialize it with defaults. | ||
| if (!hasLoadedProperties && !hasProperties) { |
There was a problem hiding this comment.
Require both configuration sources to be absent before applying defaults. This preserves values loaded from iotdb-system.properties while retaining the existing fallback when neither the system file nor an external loader is available.
jt2594838
changed the title
| // if there are no properties, we need to init memory config | ||
| if (!hasProperties) { | ||
| // If no configuration source initialized the memory config, initialize it with defaults. | ||
| if (!hasLoadedProperties && !hasProperties) { |
There was a problem hiding this comment.
Could we add a regression test that exercises this constructor branch with a real system configuration source? The 11 tests listed in the PR are unchanged from the parent commit: IoTDBDescriptorTest only checks URL resolution, while DataNodeMemoryConfigTest tests the calculation/default paths directly. As a result, removing && !hasLoadedProperties would still leave all of them passing. A test that initializes a fresh descriptor (ideally in an isolated JVM/classloader) with datanode_memory_proportion=1:1:1:1:1:5, activates the RPC buffer memory control, and verifies a maxMemory / 4 budget instead of the default maxMemory / 20 would cover the reported regression. It would also be useful to retain an assertion for the no-configuration fallback.
There was a problem hiding this comment.
Thanks for pointing this out. I added two constructor-level regression tests. One uses a real temporary iotdb-system.properties with datanode_memory_proportion=1:1:1:1:1:5 and verifies the activated RPC buffer budget is maxMemory / 4; the other verifies the no-configuration fallback remains maxMemory / 20. The tests run in separate Surefire forks so the descriptor and memory configuration statics are isolated.
| String originalConf = System.getProperty(IoTDBConstant.IOTDB_CONF); | ||
| File confDir = temporaryFolder.newFolder(); | ||
| Files.writeString( | ||
| confDir.toPath().resolve(CommonConfig.SYSTEM_CONFIG_NAME), |
There was a problem hiding this comment.
This test uses a real temporary system properties file so the constructor executes the configured-property path. It verifies the regression fix through the public RPC buffer memory control after the descriptor initializes memory from datanode_memory_proportion.
|
|
||
| @Test | ||
| public void testNoConfigurationSourceInitializesDefaultRpcBufferMemory() { | ||
| String originalConf = System.getProperty(IoTDBConstant.IOTDB_CONF); |
There was a problem hiding this comment.
This test keeps the no-configuration fallback covered by forcing the system properties lookup to return no URL. The assertion verifies that the descriptor still installs the default maxMemory / 20 RPC buffer budget when no source initializes memory.
2 participants
Description
Problem
IoTDBDescriptor initialized the DataNode memory configuration while loading iotdb-system.properties, then initialized it again with default properties whenever no external properties loader was present. The second initialization could discard configuration-derived memory settings.
Changes
Validation
This PR has:
Key changed/added classes (or packages if there are too many classes) in this PR