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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
