You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "JackieTien97 (via GitHub)" <gi...@apache.org> on 2026/01/23 04:17:35 UTC
[PR] Fix memory init bug and ddl overflow bug [iotdb]
JackieTien97 opened a new pull request, #17071:
URL: https://github.com/apache/iotdb/pull/17071
This pull request primarily addresses improvements in memory allocation and timeout handling for query execution in the DataNode module. The most significant changes include refining the calculation of memory limits per query thread, updating the initialization sequence for memory-related properties, and enhancing the robustness of timeout management for driver tasks.
**Memory Allocation Improvements:**
* The calculation of `maxBytesPerFragmentInstance` in `DataNodeMemoryConfig` is now divided by `queryThreadCount`, ensuring fairer memory distribution among query threads and preventing overallocation.
* The initialization of `queryThreadCount` from configuration properties is moved to occur after the `dataExchangeMemoryManager` is initialized, ensuring that dependent memory calculations use the correct thread count.
* The logic for setting `maxBytesPerFragmentInstance` during memory allocation initialization has been removed, as this is now handled dynamically when the query thread count is set.
* The setter for `queryThreadCount` now updates `maxBytesPerFragmentInstance` based on the current state of the memory manager, ensuring consistency if the thread count changes at runtime.
**Timeout Handling Enhancement:**
* In `DriverTask`, the computation of the deadline (`ddl`) now checks for integer overflow when adding large timeouts, assigning `Long.MAX_VALUE` if an overflow is detected to avoid infinite timeout loops during schema fetch operations.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory init bug and ddl overflow bug [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17071:
URL: https://github.com/apache/iotdb/pull/17071#discussion_r2719541596
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/schedule/task/DriverTask.java:
##########
@@ -77,7 +77,17 @@ public DriverTask(
boolean isHighestPriority) {
this.driver = driver;
this.setStatus(status);
- this.ddl = System.currentTimeMillis() + timeoutMs;
+
+ long currentTime = System.currentTimeMillis();
+ long ddlTmp = currentTime + timeoutMs;
+ // avoid infinite timeout check loop, schema fetch query for write operation may pass a very
+ // large timeout here which may causing currentTime + timeoutMs be negative
Review Comment:
The overflow check correctly detects when adding a large timeout causes integer overflow (wrapping to a negative value). However, the comment states "currentTime + timeoutMs be negative" which is slightly imprecise. The issue is not just that the result is negative, but specifically that overflow has occurred, indicated by the sum being less than currentTime. Consider clarifying the comment to say "which may cause overflow, resulting in ddlTmp being less than currentTime" for better accuracy.
```suggestion
// large timeout here which may cause overflow, resulting in ddlTmp being less than currentTime
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/DataNodeMemoryConfig.java:
##########
@@ -64,7 +64,8 @@ public class DataNodeMemoryConfig {
private int queryThreadCount = Runtime.getRuntime().availableProcessors();
/** Max bytes of each FragmentInstance for DataExchange */
- private long maxBytesPerFragmentInstance = Runtime.getRuntime().maxMemory() * 3 / 10 * 200 / 1001;
+ private long maxBytesPerFragmentInstance =
+ Runtime.getRuntime().maxMemory() * 3 / 10 * 200 / 1001 / queryThreadCount;
Review Comment:
The field initialization for maxBytesPerFragmentInstance calculates a value by dividing by queryThreadCount (initialized to the number of available processors on line 64). However, this initial value is never used because setQueryThreadCount is called during initQueryEngineMemoryAllocate (line 502-504) after dataExchangeMemoryManager is created, which recalculates maxBytesPerFragmentInstance using the actual configured value and the data exchange memory size.
The initial calculation wastes computation and could be misleading to developers who might not realize this value is immediately overwritten. Consider initializing maxBytesPerFragmentInstance to 0 and adding a comment explaining it will be set properly during initialization, or restructure the initialization to avoid the throwaway calculation.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory init bug and ddl overflow bug [iotdb]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #17071:
URL: https://github.com/apache/iotdb/pull/17071#issuecomment-3788319423
## [Codecov](https://app.codecov.io/gh/apache/iotdb/pull/17071?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `90.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 39.28%. Comparing base ([`dce185a`](https://app.codecov.io/gh/apache/iotdb/commit/dce185a745b250a572ff30479697d1a7b22fe94b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`15d3bca`](https://app.codecov.io/gh/apache/iotdb/commit/15d3bca5377cea01e73c1fd1726d3cfabb1401b0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 13 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/iotdb/pull/17071?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ueryengine/execution/schedule/task/DriverTask.java](https://app.codecov.io/gh/apache/iotdb/pull/17071?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fqueryengine%2Fexecution%2Fschedule%2Ftask%2FDriverTask.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvcXVlcnllbmdpbmUvZXhlY3V0aW9uL3NjaGVkdWxlL3Rhc2svRHJpdmVyVGFzay5qYXZh) | 80.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17071 +/- ##
============================================
- Coverage 39.28% 39.28% -0.01%
Complexity 282 282
============================================
Files 5105 5106 +1
Lines 341552 341473 -79
Branches 43535 43523 -12
============================================
- Hits 134190 134135 -55
+ Misses 207362 207338 -24
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/iotdb/pull/17071?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory init bug and ddl overflow bug [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17071:
URL: https://github.com/apache/iotdb/pull/17071#issuecomment-3788150413
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17071) **Quality Gate failed**
Failed conditions
 [5.5% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17071&metric=new_duplicated_lines_density&view=list) (required ≤ 5%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17071)
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory init bug and ddl overflow bug [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 merged PR #17071:
URL: https://github.com/apache/iotdb/pull/17071
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org