Repository navigation
[1.3] Fix query partition memory retention and failure diagnostics - #19269
JackieTien97 wants to merge 5 commits into
Conversation
JackieTien97
left a comment
There was a problem hiding this comment.
Reviewed at 856d4a0d82 (base 013622816a). I built base, 33277a6cf5, and this head and ran each as a 1C1D cluster (Windows, JDK 21, dn_thrift_max_frame_size=1048576, 64 devices with one point each). The inline numbers come from these clusters and from direct transport probes against the same jars.
Verified on this head
- The oversized partition request (
select s1 from root.db.** where time >= 0 and time < 3000000000000, ≈3.8 MB) now fails in 35 ms withFrame size (1048582) larger than protect max size (1048576)!. At33277a6cf5and on base it took 15.2 s (15 reconnect attempts) and reported "Fail to connect to any config node", even with a 3 s JDBC query timeout. - Compressed transport, two cases. At
33277a6cf5both escaped asIllegalArgumentException, and the flush case left the transport open. Both now raiseTTransportExceptionand close the transport:- a
maxCompressedLengthoverflow inflush()(forced 1.9 GB frame); - a varint length ≥ 2^31 in
readFrame().
- a
- A near-unbounded range (
time >= 0 and time <= 9223372036854775806) now fails with 1707 "Not enough memory while analyzing time partitions … requested this time 340.87 GB …, current free memory 772.99 MB". - The growth fix and the deadline checks look correct.
startTimeis only set byCoordinator, so standalone contexts are unaffected. - All 56 tests in the 7 touched test classes pass locally:
ConfigNodeClientTest2,MPPQueryContextTest7,QueryPartitionMemoryTest5,QueryTimePartitionTest16,LocalExecutionPlannerOperatorsMemoryTest12,AutoResizingBufferTest9,TElasticFramedTransportTest5.
Comments
- [P2]
Coordinator: the new[QueryFailed]WARN logs every analysis/planning failure with a stack trace. That includes user errors thatErrorHandlingUtilskeeps at INFO, and every other failure ends up logged twice. - [P3]
TElasticFramedTransport#write: buffer growth still overshoots the limit by 1.30–1.44×, and the error now reports the size at the crossing write instead of the request size.
Nit: sonar-codecov.yml. codecov has failed on the last five dev/1.3 pushes with the same spotless NoSuchMethodError (e.g. run 43390944744), so this JDK pin fixes the branch, not just this PR. It may deserve its own commit or PR so the fix survives if this PR is reworked.
| } catch (RuntimeException | Error e) { | ||
| // Analysis can fail before the execution is registered, so later error handling cannot look | ||
| // up its SQL in queryExecutionMap. Keep the SQL and query ID in the failure log here. | ||
| LOGGER.warn("[QueryFailed] queryId: {}, sql: {}", globalQueryId, sql, e); |
There was a problem hiding this comment.
[P2] This logs every analysis/planning failure at WARN with a stack trace, including user errors that ErrorHandlingUtils deliberately keeps at INFO
ErrorHandlingUtils#onQueryException already logs these failures:
- user-caused ones (
SQL_PARSE_ERROR,SEMANTIC_ERROR,NO_PERMISSION,QUERY_TIMEOUT, …) at INFO with no stack trace; - everything else with the stack.
On the client path that line already includes the SQL: Query Statement: "<sql>". executeStatement failed because …. This catch runs before that classification. DataNode log on this head, client statements via JDBC:
| statement | existing ErrorHandlingUtils line |
new [QueryFailed] line |
|---|---|---|
select count(s1), s1 from root.db.d0 → 701 |
INFO, no stack | WARN + SemanticException stack (23 frames) |
| oversized partition request → 305 | WARN + stack | WARN + the same stack again |
select s1 from root.db.d0 where time >= 0 and time <= 9223372036854775806 → 1707 |
WARN + stack | WARN + the same stack again |
The consequences:
- A dashboard that keeps sending an invalid query now writes a WARN stack trace on every execution.
- So does a query that hits the new analysis-time deadline check (
QueryTimeoutRuntimeException). - On the client path, every other failure that throws out of
execution()is logged twice.
If the goal is to tie the query ID to the SQL when a failure happens before registration, two options:
- Log this line without the throwable, since callers already log it.
- Skip the classes that
ErrorHandlingUtilstreats as user errors (at leastSemanticExceptionandQueryTimeoutRuntimeException), or log those at DEBUG.
assertFailureLogs in QueryPartitionMemoryTest would need to follow whichever option is chosen.
| close(); | ||
| throw new TTransportException( | ||
| TTransportException.CORRUPTED_DATA, | ||
| "Frame size (" + size + ") larger than protect max size (" + maxSize + ")!"); |
There was a problem hiding this comment.
[P3] The reported size is now just past the limit, not the request size
For the same 64-device query:
- base logged
Frame size (3811712) larger than protect max size (1048576)!; - this head logs
Frame size (1048582) larger than protect max size (1048576)!, which is also what the client now sees in the 305 message.
The old number showed how far over the limit the request was, which is what an operator needs in order to size dn_thrift_max_frame_size. The full size isn't known at this point anymore. Consider wording the message as "at least N bytes" and naming the setting.
| TTransportException.CORRUPTED_DATA, | ||
| "Frame size (" + size + ") larger than protect max size (" + maxSize + ")!"); | ||
| } | ||
| writeBuffer.write(buf, off, len); |
There was a problem hiding this comment.
[P3] Growth can still overshoot the limit before the rejection
writeBuffer.write grows the buffer by 1.5× without knowing getMaxWriteBufferSize(). The last growth step before the rejection can therefore allocate well past the limit. Measured on this head with 8-byte writes and a 1024-byte initial buffer:
| limit | capacity at rejection |
|---|---|
| 1 MiB | 1,513,342 (1.44×) |
| 64 MiB (DataNode default for heaps ≥ 4 GiB) | 87,266,902 (1.30×) |
Passing the cap down to the write buffer would keep the array at or below the limit, e.g. min(grow, maxSize) when growing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.3 #19269 +/- ##
=============================================
+ Coverage 45.56% 47.47% +1.91%
- Complexity 198 221 +23
=============================================
Files 3667 3678 +11
Lines 251340 252115 +775
Branches 31156 31359 +203
=============================================
+ Hits 114529 119699 +5170
+ Misses 136811 132416 -4395 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|



Description
Very large time-partition requests can keep query analysis running while retaining reservations from the operator memory pool. The Thrift buffer's 1.5x growth overflowed
intabove roughly 1.33 GiB, causing an entire large array to be copied for every small field write. The uncompressed frame limit was checked only after serialization, atflush().The change makes these failures bounded and diagnosable:
Reproduction and validation
On the base revision
013622816a, a real 1,500,000,000-byte buffer grew by only 8 bytes for each of three successive writes, copying the entire array each time. With the fix, it grows once and subsequent writes reuse the buffer. This large-array check runs separately with a bounded 5 GiB heap; ordinary unit tests do not require multi-gigabyte allocations.Regression tests exercise the real SQL parser, Coordinator, Analyzer, and generated ConfigNode Thrift serializer with test schema/partition fetchers and in-memory transports. Repeated oversized requests and partition-memory failures fully restore earlier operator-pool reservations. Tests verify INFO context records include the SQL and query ID for memory shortages, semantic errors, timeouts, and frame-size failures without an attached throwable, and that normal queries do not produce failure logs. Incremental 8-byte writes prove a 1 MiB cap never allocates beyond 1 MiB; initial capacities above the configured cap are also covered. A simulated OutOfMemoryError test verifies that SQL context is recorded and the original error is rethrown unchanged while earlier reservations are released.
84 distinct tests passed in a clean reactor build on JDK 21 (Java 17 target), covering normal and templated analysis, query retry, memory accounting, diagnostics, ConfigNode retry behavior, compressed/uncompressed frames, and buffer reuse/shrinking. Checkstyle and Spotless passed. The coverage job now explicitly selects JDK 21, matching Sonar, because the runner default JDK caused google-java-format to fail before any coverage tests could run.
mvn -B clean test -pl iotdb-core/datanode -am \ -Dtest=AutoResizingBufferTest,TElasticFramedTransportTest,RpcUtilsTest,UrlUtilsTest,TSStatusCodeTest,QueryTimePartitionTest,QueryPartitionMemoryTest,AnalyzeTest,QueryExecutionRetryTest,MPPQueryContextTest,LocalExecutionPlannerOperatorsMemoryTest,ConfigNodeClientTest,TemplatedAnalyzeRetryTest \ -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=falseThe local reproductions use controlled inputs and in-memory transports; they do not replay the original production SQL or start a live cluster.