Skip to content

Optimize the configuration logic of dn_thrift_max_frame_size#16724

Merged
jt2594838 merged 3 commits intomasterfrom
change_thrift_max_frame_size
Nov 10, 2025
Merged

Optimize the configuration logic of dn_thrift_max_frame_size#16724
jt2594838 merged 3 commits intomasterfrom
change_thrift_max_frame_size

Conversation

@HTHou
Copy link
Contributor

@HTHou HTHou commented Nov 10, 2025

Description

  • Set the default value to 0 and automatically calculate it based on the JVM configuration parameters when DN starts, with the value being min(64MB, dn_alloc_memory / 64)
  • if the user manually configures this parameter, the size specified by the user will be prioritized.

@HTHou HTHou requested a review from Copilot November 10, 2025 07:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the Thrift max frame size configuration to use dynamic memory-based defaults and consolidates frame validation error handling.

Key Changes

  • Removed hardcoded LEFT_SIZE_IN_REQUEST constant and related validation logic
  • Changed default dn_thrift_max_frame_size from 512MB to 0 (dynamic), which calculates as min(64MB, heap memory / 64)
  • Refactored frame size validation in TElasticFramedTransport using an enum-based error handling pattern

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
IoTDBConstant.java Removed unused LEFT_SIZE_IN_REQUEST constant (4MB)
iotdb-system.properties.template Updated default value to 0 and clarified documentation for dynamic calculation
IoTDBDescriptor.java Removed minimum frame size validation that enforced 8MB minimum
IoTDBConfig.java Implemented dynamic calculation of thriftMaxFrameSize based on heap memory, with fallback logic in setter
TElasticFramedTransport.java Refactored frame validation into checkFrameSize() method with enum-based error handling for better code organization
TCompressedElasticFramedTransport.java Simplified by delegating frame size validation to parent class's checkFrameSize() method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

❌ Patch coverage is 84.44444% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.75%. Comparing base (9d4c410) to head (a7b9b83).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
.../org/apache/iotdb/rpc/TElasticFramedTransport.java 84.61% 6 Missing ⚠️
...e/iotdb/rpc/TCompressedElasticFramedTransport.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #16724      +/-   ##
============================================
- Coverage     38.75%   38.75%   -0.01%     
  Complexity      207      207              
============================================
  Files          5001     5001              
  Lines        331342   331356      +14     
  Branches      42103    42102       -1     
============================================
+ Hits         128421   128426       +5     
- Misses       202921   202930       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jt2594838 jt2594838 merged commit ffa9c56 into master Nov 10, 2025
34 of 35 checks passed
@HTHou HTHou deleted the change_thrift_max_frame_size branch November 10, 2025 10:17
JackieTien97 pushed a commit that referenced this pull request Nov 26, 2025
* Add thrift max frame size calculate logic

* Add thrift max frame size calculate logic

* fix review

(cherry picked from commit ffa9c56)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants