You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "Wei-hao-Li (via GitHub)" <gi...@apache.org> on 2026/09/16 02:15:44 UTC
[PR] Fragment large TsBlocks in MPP DataExchange responses [iotdb]
Wei-hao-Li opened a new pull request, #18645:
URL: https://github.com/apache/iotdb/pull/18645
(no comment)
--
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] Fragment large TsBlocks in MPP DataExchange responses [iotdb]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #18645:
URL: https://github.com/apache/iotdb/pull/18645#issuecomment-5708536159
## [Codecov](https://app.codecov.io/gh/apache/iotdb/pull/18645?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 `75.16340%` with `38 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 42.85%. Comparing base ([`25ab794`](https://app.codecov.io/gh/apache/iotdb/commit/25ab7941cebe6e14f396b5541cdd9daefae0f151?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4b404ae`](https://app.codecov.io/gh/apache/iotdb/commit/4b404aed1654722b7273acef31fc877e4895d88f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 32 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/iotdb/pull/18645?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ine/execution/exchange/MPPDataExchangeManager.java](https://app.codecov.io/gh/apache/iotdb/pull/18645?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fqueryengine%2Fexecution%2Fexchange%2FMPPDataExchangeManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvcXVlcnllbmdpbmUvZXhlY3V0aW9uL2V4Y2hhbmdlL01QUERhdGFFeGNoYW5nZU1hbmFnZXIuamF2YQ==) | 0.00% | [29 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/18645?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...engine/execution/exchange/source/SourceHandle.java](https://app.codecov.io/gh/apache/iotdb/pull/18645?src=pr&el=tree&filepath=iotdb-core%2Fdatanode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fdb%2Fqueryengine%2Fexecution%2Fexchange%2Fsource%2FSourceHandle.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9kYXRhbm9kZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaW90ZGIvZGIvcXVlcnllbmdpbmUvZXhlY3V0aW9uL2V4Y2hhbmdlL3NvdXJjZS9Tb3VyY2VIYW5kbGUuamF2YQ==) | 86.15% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/18645?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 #18645 +/- ##
============================================
+ Coverage 42.68% 42.85% +0.16%
Complexity 442 442
============================================
Files 5441 5451 +10
Lines 393714 395699 +1985
Branches 51597 51851 +254
============================================
+ Hits 168051 169564 +1513
- Misses 225663 226135 +472
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/iotdb/pull/18645?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] Fragment large TsBlocks in MPP DataExchange responses [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 commented on code in PR #18645:
URL: https://github.com/apache/iotdb/pull/18645#discussion_r4033824988
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/exchange/source/SourceHandle.java:
##########
@@ -767,6 +779,115 @@ private void fail(Throwable t) {
sourceHandleListener.onFailure(SourceHandle.this, t);
}
}
+
+ private TGetDataBlockResponse getDataBlockWithFragments(
+ SyncDataNodeMPPDataExchangeServiceClient client,
+ TGetDataBlockRequest request,
+ DataBlockFetchProgress fetchProgress)
+ throws TException {
+ while (!fetchProgress.isFinished()) {
+ TGetDataBlockRequest fragmentRequest = request.deepCopy();
+ fragmentRequest.setStartSequenceId(fetchProgress.nextSequenceId);
+ fragmentRequest.setOffset(fetchProgress.offset);
+ TGetDataBlockResponse response = client.getDataBlock(fragmentRequest);
Review Comment:
[P2] Stop fetching fragments when the source is closed or aborted
If `close()` or `abort()` runs while a fragment RPC is in flight, this loop still consumes its response and downloads the remaining fragments because it only checks `fetchProgress.isFinished()`. The source has already returned its reservation to the query pool, so it can allocate the full `PublicBAOS(totalLength)` and retain accumulated data after cancellation without an accounted reservation. A focused test that closes or aborts during the first RPC of a three-fragment transfer still executes all three RPCs with `getBufferRetainedSizeInBytes() == 0`. For large blocks this also keeps the exchange worker and connection occupied until the entire range is fetched. Please check the terminal state with appropriate synchronization before each new RPC and again before consuming its response, and discard the pending fetch state when stopping.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java:
##########
@@ -3083,6 +3092,41 @@ public void loadShuffleProps(TrimProperties properties) {
Integer.toString(commonConfig.getDriverTaskExecutionTimeSliceInMs()))));
}
+ private void loadMppDataExchangeMaxPayloadSize(TrimProperties properties) {
+ int configuredSize =
+ Integer.parseInt(
+ properties.getProperty(
+ "mpp_data_exchange_max_payload_size_in_bytes",
+ Integer.toString(conf.getMppDataExchangeMaxPayloadSizeInBytes())));
+ if (configuredSize <= 0) {
+ LOGGER.warn(
+ DataNodeMiscMessages
+ .LOG_MPP_DATA_EXCHANGE_MAX_PAYLOAD_SIZE_ARG_IS_NOT_POSITIVE_USING_DEFAULT_VALUE_ARG_1AA821B2,
+ configuredSize,
+ DEFAULT_MPP_DATA_EXCHANGE_MAX_PAYLOAD_SIZE_IN_BYTES);
Review Comment:
[P3] Use the correct placeholders for the configuration warnings
The new message constants contain `%d`, but `LOGGER.warn(template, args...)` performs SLF4J `{}` substitution. Consequently this warning is emitted as `mpp_data_exchange_max_payload_size_in_bytes (%d) is not positive, using default value %d`, without the configured or effective values. The below-minimum and above-maximum warnings have the same problem in both locales. Please use `{}` in the English and Chinese templates, or explicitly apply `String.format()` before logging, so operators can see which values were rejected and applied.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/exchange/source/SourceHandle.java:
##########
@@ -709,7 +714,14 @@ public void run() {
return;
}
break;
- } catch (Throwable e) {
+ } catch (IllegalArgumentException | IllegalStateException e) {
+ if (!transferAttemptRecorded) {
+ recordTransferAttempt(
+ false, UserDataTransferErrorCode.UNEXPECTED_RESPONSE_SIZE.name(), e);
+ }
+ fail(e);
+ return;
+ } catch (Exception e) {
Review Comment:
[P2] Preserve failure cleanup for errors during fragment assembly
Narrowing the previous `catch (Throwable)` to `catch (Exception)` leaves an `OutOfMemoryError` from the new `PublicBAOS(totalLength)` allocation uncaught. This allocation happens after `client.getDataBlock()` returns, so the Thrift client wrapper cannot convert it into a `TException`. The submitted task then terminates without calling `fail()`: the executor only logs the error, leaving the source blocked and its reservation held until an outer timeout or cancellation. Using the JVM array-size limit to safely inject an allocation error reproduced zero failure callbacks, a blocked source, and a retained 1,024-byte reservation. Please also notify the fragment of failure and release the reservation on the `Error` path; failing immediately is sufficient and does not require retrying an OOM.
--
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] Fragment large TsBlocks in MPP DataExchange responses [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 merged PR #18645:
URL: https://github.com/apache/iotdb/pull/18645
--
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] Fragment large TsBlocks in MPP DataExchange responses [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 commented on code in PR #18645:
URL: https://github.com/apache/iotdb/pull/18645#discussion_r4033825000
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java:
##########
@@ -3083,6 +3092,41 @@ public void loadShuffleProps(TrimProperties properties) {
Integer.toString(commonConfig.getDriverTaskExecutionTimeSliceInMs()))));
}
+ private void loadMppDataExchangeMaxPayloadSize(TrimProperties properties) {
+ int configuredSize =
+ Integer.parseInt(
+ properties.getProperty(
+ "mpp_data_exchange_max_payload_size_in_bytes",
+ Integer.toString(conf.getMppDataExchangeMaxPayloadSizeInBytes())));
+ if (configuredSize <= 0) {
+ LOGGER.warn(
+ DataNodeMiscMessages
+ .LOG_MPP_DATA_EXCHANGE_MAX_PAYLOAD_SIZE_ARG_IS_NOT_POSITIVE_USING_DEFAULT_VALUE_ARG_1AA821B2,
+ configuredSize,
+ DEFAULT_MPP_DATA_EXCHANGE_MAX_PAYLOAD_SIZE_IN_BYTES);
Review Comment:
[P3] Use the correct placeholders for the configuration warnings
The same placeholder mismatch occurs at all three warning calls in `loadMppDataExchangeMaxPayloadSize()`:
1. [Non-positive configured value, lines 3102–3106](https://github.com/apache/iotdb/blob/e794159c67216f63461d71e073b39e7aea76396d/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java#L3102-L3106).
2. [Below the minimum value, lines 3109–3114](https://github.com/apache/iotdb/blob/e794159c67216f63461d71e073b39e7aea76396d/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java#L3109-L3114).
3. [Above the maximum value, lines 3119–3124](https://github.com/apache/iotdb/blob/e794159c67216f63461d71e073b39e7aea76396d/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java#L3119-L3124).
All three new message constants contain `%d`, but `LOGGER.warn(template, args...)` performs SLF4J `{}` substitution. For example, the first warning is emitted as `mpp_data_exchange_max_payload_size_in_bytes (%d) is not positive, using default value %d`, without the configured or effective values. Both the English and Chinese templates are affected. Please use `{}` in the templates, or explicitly apply `String.format()` before logging, so operators can see which values were rejected and applied.
--
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