Repository navigation
[IT] Pin the DataRegions per DataNode in IoTDBMultiTAGsWithAttributesTableIT - #19086
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19086 +/- ##
============================================
+ Coverage 48.43% 48.47% +0.03%
Complexity 635 635
============================================
Files 5572 5572
Lines 412012 412044 +32
Branches 54309 54316 +7
============================================
+ Hits 199556 199724 +168
+ Misses 212456 212320 -136 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The failing SonarCloud quality gate is not caused by this change. Its only bug finding is S3078 ("Use an AtomicBoolean") in |
|
The failing SonarCloud quality gate (new_reliability_rating = 3) is not caused by this change: it comes from two bugs in recently merged pipe code and currently fails for every pull request. #19109 fixes them (its own SonarCloud gate passes); once it is merged, the gate here should pass on the next analysis. |
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. I agree with what fails: fourTableJoinTest creates more FragmentInstances than the operator memory of a 256 MB DataNode can hold. But parts of the root-cause write-up don't match the current code, and I think pinning the region count would be a more robust fix than scaling the heap.
1. The default extension policy is PROACTIVE, not AUTO.
Since #18692, data_region_group_extension_policy defaults to PROACTIVE (ConfigNodeConfig L156-L157). PROACTIVE creates one DataRegionGroup per active series slot, up to the same maximum as AUTO, which is 0.5 × cores here (PartitionManager L670-L684). That is what makes the region count follow the CPU cores in this test.
Under AUTO, the extension branch (L765-L766) only adds groups when slotCount / allocated > 1000 / max. With a maximum of 6 groups, that means more than about 333 slots. This test writes only about 40 devices, so under AUTO it would stay at the minimum of 2 groups on practically any machine.
So this most likely started with #18692 rather than being a long-standing many-core issue. Other ITs that write several devices and reserve memory per FragmentInstance may hit the same failure locally. Could you correct the commit message and the description accordingly?
2. CI already runs this class with a 1 GB DataNode heap.
Every workflow that runs this class passes -DDataNodeMaxHeapSize=1024:
That gives CI about 64 MB of operator memory, which is enough even with 6 or more regions. With this change, Math.max(1024, regions * 128) does nothing on CI runners with 16 cores or fewer. On the self-hosted runners (1C3D with forkCount=6, and the daily jobs) it raises the heap above 1 GB if they have more than 16 cores. So "CI runs with the same settings as before" holds only for the small runners. The real difference between the local failure and CI is the missing heap flag, combined with PROACTIVE.
3. Suggestion: pin the number of DataRegions per DataNode instead of scaling the heap.
EnvFactory.getEnv().getConfig().getDataNodeCommonConfig().setSortBufferSize(1024 * 1024L);
// Pin the DataRegions per DataNode so that the region layout (and so the FragmentInstances of
// fourTableJoinTest) matches a 4-core CI runner instead of following the host's CPU cores.
EnvFactory.getEnv().getConfig().getCommonConfig().setDataRegionPerDataNode(2);
EnvFactory.getEnv().initClusterEnvironment();When data_region_per_data_node is set, the maximum becomes 2 × DataNodes instead of 0.5 × cores (ClusterSchemaManager L619-L636). That gives 2 regions in 1C1D and 6 in 1C3D on every machine, the same layout as a 4-core runner, and needs no heap formula. Switching to setDataRegionGroupExtensionPolicy("CUSTOM") would also pin the count, but then the count comes from default_data_region_group_num_per_database, and 1C3D drops to 2 regions.
I understand wanting to exercise more regions on bigger machines, but for this test the extra regions add little. The four joined tables have only 5/3/5/5 devices, all in a single time partition. So the number of scan FragmentInstances stops growing at about 18, while the heap formula keeps growing linearly: 4 GB per DataNode, or 12 GB for 1C3D, on a 64-core host. If you prefer to keep the region layout host-dependent, a fixed 1 GB heap like CI's should already cover that limit.
A pre-existing issue, not introduced here: setMaxTsBlockLineNumber(2).setMaxNumberOfPointsInPage(5) is applied after initClusterEnvironment(), so these settings never reach the running nodes (see the inline comment).
| public class IoTDBMultiTAGsWithAttributesTableIT { | ||
| private static final String DATABASE_NAME = "db"; | ||
| // Default heap of the test DataNodes (see DataNodeWrapper#initVMConfig). | ||
| private static final int DEFAULT_DATANODE_MAX_HEAP_SIZE_IN_MB = 256; |
There was a problem hiding this comment.
This duplicates the literal and the system-property lookup in DataNodeWrapper#initVMConfig (L178). MppJVMConfig.override() replaces any positive value and doesn't re-check init <= max (L89-L97). So if the framework default ever changes (for example, aligned with CI's 1024 MB, or with a larger init heap), this test would quietly lower the heap on small machines, or produce -Xms > -Xmx and the DataNodes would fail to start. If the heap override stays, it would be safer to override only when the computed value is larger than the effective default, or to get the effective default from the framework instead of copying it.
| private static final int DEFAULT_DATANODE_MAX_HEAP_SIZE_IN_MB = 256; | ||
| // fourTableJoinTest needs about 3.5 MB of operator memory per DataRegion. A 256 MB heap has | ||
| // about 16 MB of it, enough for about 4 regions; 128 MB of heap per region leaves a 2x margin. | ||
| private static final int DATANODE_HEAP_SIZE_PER_DATA_REGION_IN_MB = 128; |
There was a problem hiding this comment.
The per-region model doesn't hold beyond a few regions. students/teachers/courses/grades have 5/3/5/5 devices, all in one time partition, so the number of scan FragmentInstances of fourTableJoinTest is bounded by those 18 devices, however many regions exist. The heap has no bound, though. On 64 cores it is 32 × 128 MB = 4 GB per DataNode, or 12 GB of -Xmx for 1C3D, where each DataNode only runs about a third of the scans anyway. If this approach stays, please cap it, for example at 1024 MB, which is what CI uses.
| // the DataNode heap, and so its operator memory, with the regions. On a 4-core machine | ||
| // (2 regions) this keeps the default heap. | ||
| final int expectedDataRegionsPerDataNode = | ||
| Math.max(1, Runtime.getRuntime().availableProcessors() / 2); |
There was a problem hiding this comment.
Minor: this re-derives the ConfigNode's formula, and it already differs from it in three ways:
- The ConfigNode rounds up (
Math.ceil(...), ClusterSchemaManager L595-L598). - It uses the cores that each DataNode JVM reports (DataNode L1027), not the test JVM's.
- It honors
data_region_per_data_nodeoverrides, for example from-DClusterConfigurations.
On odd core counts, this underestimates by one region and relies on the 2x margin. That's another reason to pin the region count instead (see the review summary).
| DEFAULT_DATANODE_MAX_HEAP_SIZE_IN_MB, | ||
| 0), | ||
| expectedDataRegionsPerDataNode * DATANODE_HEAP_SIZE_PER_DATA_REGION_IN_MB)); | ||
| EnvFactory.getEnv().initClusterEnvironment(); |
There was a problem hiding this comment.
Pre-existing, and not caused by this PR, but since this setUp is being touched anyway: the setMaxTsBlockLineNumber(2).setMaxNumberOfPointsInPage(5) call just below runs after initClusterEnvironment(). The node configuration is only written when the nodes are created, so these two settings never reach the running cluster, and the test has always run with the defaults. Moving them before initClusterEnvironment() would apply them, but it changes the effective configuration (and possibly the memory numbers above), so it may be better as a separate change.
|
hi chris @chrisdutz , you can fix these review comments when you've got time |
…TableIT Since #18692, the default DataRegionGroup extension policy is PROACTIVE, which creates DataRegionGroups up to 0.5 per CPU core of the DataNodes: 2 on a 4-core CI runner, 6 on a 12-core machine. The number of fragment instances of fourTableJoinTest grows with it, and many of them reserve about 1 MB of operator memory because of the 1 MB sort buffer the test sets. With 6 regions, the 25 fragment instances don't fit into the about 16 MB of operator memory of the default 256 MB DataNode heap, and the test fails with "There is not enough memory to execute current fragment instance". CI passes -DDataNodeMaxHeapSize=1024, which hides it there. Set data_region_per_data_node to 2, so that the maximum is 2 regions per DataNode instead of 0.5 per core: the same layout as on a 4-core runner (2 regions in 1C1D, 6 in 1C3D) on every machine, with the default heap.
|
Hi Jackie, Chris |
095124b to
c77daf1
Compare
chrisdutz
changed the title
|
Thanks for the thorough review, you're right on all points. I've reworked the PR:
I've left the |
|
Description
Problem
On machines with many CPU cores,
IoTDBMultiTAGsWithAttributesTableIT.fourTableJoinTestfails every time when run with the default DataNode heap:Cause
Since #18692, the default
data_region_group_extension_policyisPROACTIVE. It creates DataRegionGroups up to0.5 × coresof the DataNodes, so the region count of this test follows the CPU cores. The fragment instances of the four-table join grow with it, and many of them reserve about 1 MB of operator memory because the test sets a 1 MB sort buffer. A 256 MB DataNode heap has about 16 MB of operator memory:fourTableJoinTestCI doesn't see it because every workflow running this class passes
-DDataNodeMaxHeapSize=1024.Fix
Pin
data_region_per_data_nodeto 2 in the test's setup (suggested by @JackieTien97). The maximum then becomes2 × DataNodesinstead of0.5 × cores: 2 regions in 1C1D and 6 in 1C3D on every machine, the same layout as on a 4-core runner, with the default heap.The setting takes precedence over
-DintegrationTest.dataRegionPerDataNodefor this class.The earlier version of this PR scaled the DataNode heap with the cores instead; that is dropped.
Verification
IoTDBMultiTAGsWithAttributesTableITalone, on macOS with 12 cores, default DataNode heap (256 MB):fourTableJoinTestfailsTableSimpleITTableClusterIT(1C3D)spotless:checkpasses forintegration-test.This PR has:
Key changed/added classes (or packages if there are too many classes) in this PR
integration-test/src/test/java/org/apache/iotdb/relational/it/db/it/IoTDBMultiTAGsWithAttributesTableIT.java🤖 Generated with Claude Code