Skip to content

Avoid duplicate DataNode memory configuration initialization #18467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jt2594838 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid duplicate DataNode memory configuration initialization #18467

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public class IoTDBDescriptor {
}

protected IoTDBDescriptor() {
loadProps();
boolean hasLoadedProperties = loadProps();

@jt2594838 jt2594838 Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ServiceLoader<IPropertiesLoader> propertiesLoaderServiceLoader =
ServiceLoader.load(IPropertiesLoader.class);
boolean hasProperties = false;
Expand All @@ -167,8 +167,8 @@ protected IoTDBDescriptor() {
.getConfig()
.setCustomizedProperties(loader.getCustomizedProperties());
}
// 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) {

@jt2594838 jt2594838 Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Caideyipi Caideyipi Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

memoryConfig.init(new TrimProperties());
}
}
Expand Down Expand Up @@ -227,7 +227,7 @@ else if (!urlString.endsWith(".properties")) {

/** load a property file and set TsfileDBConfig variables. */
@SuppressWarnings("squid:S3776") // Suppress high Cognitive Complexity warning
private void loadProps() {
private boolean loadProps() {

@jt2594838 jt2594838 Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TrimProperties commonProperties = new TrimProperties();
// if new properties file exist, skip old properties files
URL url = getPropsUrl(CommonConfig.SYSTEM_CONFIG_NAME);
Expand Down Expand Up @@ -256,11 +256,13 @@ private void loadProps() {
.getMetricConfig()
.updateRpcInstance(NodeType.DATANODE, SchemaConstant.SYSTEM_DATABASE);
}
return true;
} else {
LOGGER.warn(
DataNodeMiscMessages
.MISC_LOG_COULDN_T_LOAD_THE_CONFIGURATION_FROM_ANY_OF_THE_KNOWN_SOURCES_EE3ED103,
CommonConfig.SYSTEM_CONFIG_NAME);
return false;
}
}

Expand Down