Repository navigation
Conversation
| if (chooseMemTablesToMarkFlush(tsFileProcessor)) { | ||
| boolean currentProcessorSelected = chooseMemTablesToMarkFlush(tsFileProcessor); | ||
| if (!currentProcessorSelected | ||
| && tsFileProcessor != null | ||
| && tsFileProcessor.getWorkMemTable() != null) { | ||
| // The writer holds its region's write lock. Waiting for another processor in that region | ||
| // to flush would prevent the flush submitter from acquiring the same lock. Let this writer | ||
| // finish and flush its own memtable, subject to the hard memory limit below. | ||
| tsFileProcessor.setWorkMemTableShouldFlush(); | ||
| flushTaskSubmitThreadPool.submit(tsFileProcessor::submitAFlushTask); | ||
| currentProcessorSelected = true; | ||
| } | ||
| if (currentProcessorSelected) { |
There was a problem hiding this comment.
This may result in flushing many small files.
There was a problem hiding this comment.
Agreed, this is a valid risk and should be addressed before merging. The fallback can flush the current processor even when its memtable is much smaller than the processors selected by the normal policy. The workload report shows 172/280/458 reject-status events and zero rejected inserts/606 errors, but it does not include flush counts or memtable/file-size distribution, so it cannot rule out small-file amplification. I will treat this as unresolved and collect those metrics under the same workload before claiming the fallback has no such cost.
There was a problem hiding this comment.
Follow-up: I reproduced a separate RPC regression in fa77c9350c2. The framed-transport flush reset passes the initial buffer size to resizeIfNecessary; with this PR's new shrink eligibility for requests at or below the initial size, repeated large frames shrink a 1,000-byte write buffer to 550 bytes after the cooldown/count, then require it to grow again. Under a shared memory cap, that churn can make another live buffer's allocation fail. An identical four-case harness passes against the pre-PR parent, fails three cases against fa77c9350c2, and passes all four with a local fix that shrinks from the completed frame size and only grows during incremental serialization. The local service-rpc suite passes (51 English tests; 18 Chinese tests), as do 25 focused storage tests.
This is related, but it does not explain all errors in REPORT.md: parsing the receiver logs shows 6,833 / 4,276 / 11,480 distinct allocation-failure events, all but 21 on node 46 are read-buffer frame growth failures. Node 46 also has 21 constructor failures for 1 KiB initial buffers, which indicates the shared 819 MiB RPC budget was exhausted. The PR's unnecessary write-buffer churn can add pressure, while the existing read failures still need concurrent-budget investigation. This thread remains open pending that work.
There was a problem hiding this comment.
Pushed ab56b84 with the RPC capacity fix and protection against repeated flush submission.
The fallback is retained: the inserting thread holds the region write lock, so waiting for another processor's flush can prevent that flush submitter from acquiring the same lock. Both the normal selection and fallback now reuse an already marked memtable's pending flush instead of enqueueing another task on every report. A replacement memtable can still be scheduled. Tests cover small/first-tablet memtables, repeated reports, normal largest-first selection, rejection recovery, and rejection at and above the hard limit.
This does not establish that the small-file risk is eliminated. DirectFlushPolicy still closes the selected TsFileProcessor, and legitimately selecting a small current memtable can still produce a small file. The existing close-future guards make repeated close requests idempotent, but do not remove that policy tradeoff. A matched old/new workload comparison of flush counts, memtable sizes, file sizes, and write progress is still required; I am leaving this discussion unresolved.
Separately, incremental RPC writes now only ensure capacity; both framed transports make their shrink decision using the completed frame size. The optional-growth fallback to the required capacity and failure accounting remain covered. Against the original PR, the strengthened buffer suite reproduced 3 failing tests and SystemInfoTest reproduced 4; after the fix, service-rpc verify passed all 108 English tests, the Chinese buffer suite passed 19, and SystemInfoTest plus TsFileProcessorTest passed 36. Spotless, Checkstyle, and the verify lifecycle checks passed.
I also rechecked REPORT.md's raw receiver logs: 6,833 / 4,276 / 11,478 distinct failures; all are read-buffer growth failures except 21 initial 1 KiB read-buffer allocation failures on node 46. Startup logs show a shared RPC budget of 858,993,459 bytes. A controlled read-budget rejection/release/retry test passes with the buffer and memory-account implementations from before the current branch's last six commits as well as the original PR. This supports budget exhaustion as the immediate mechanism, but neither that test nor the logs establish which change caused the workload's pressure. The independently reproduced write shrink/grow regression is fixed; eliminating all workload read-allocation failures remains unverified.
|
Correction after parsing the raw receiver error logs: REPORT.md counts both the outer TTransportException and its Caused by line, so distinct failures are 6,833 (node 44), 4,276 (45), and 11,478 (46). The stacks identify 6,833/4,276/11,457 as read-buffer growth failures in AutoScalingBufferReadTransport.fill -> TElasticFramedTransport.readFrame; node 46 has 21 additional failures allocating the initial 1 KiB buffer. I found no write-buffer allocation failures. The failed read-frame allocations have median sizes of about 13.2/14.0/13.8 MB, with a maximum near 31.9 MB. This shows shared RPC-budget exhaustion during concurrent reads, but does not by itself establish that this PR caused those read failures. The write-buffer shrink/grow regression remains independently reproducible and may add contention on that shared budget. For the report window 11:44:39-12:29:06, reject-status events are 170/0/210 for nodes 44/45/46. The totals 172/280/458 extend beyond that window; node 45 first reports one at 12:29:11. Compression logs within the same window show 3/1/13 files below 1 MiB and 37/3/80 below 10 MiB. Without a same-workload old-build baseline, these counts do not quantify PR-induced small-file amplification. I recommend holding approval until the write-buffer regression is fixed and the fallback flush is compared with aligned flush/file-size metrics. This remains unresolved. |
jt2594838
left a comment
There was a problem hiding this comment.
Revert the flush-triggering condition.
|
Applied in commit 346d355. I reverted the reject-path fallback that marked and submitted the current processor when the normal largest-first selection did not choose it. The duplicate flush-submission guard remains, as do the RPC frame-buffer fixes. SystemInfoTest now verifies that the fallback cannot bypass selection, while still covering replacement memtables and the hard memory limit when the current processor is selected. Validation: SystemInfoTest 10/10 and AutoResizingBufferTest 19/19 passed. I am leaving this discussion unresolved. |
Description
Large tablet transfers can encounter receiver stalls in two memory-control paths:
Evidence and remaining validation
The raw REPORT.md receiver logs contain 6,833 / 4,276 / 11,478 distinct allocation failures. All are on read-buffer growth except 21 initial 1 KiB read-buffer allocations on node 46. They show shared RPC-budget exhaustion, not proof that the PR's write-buffer regression caused those failures. The write regression is independently reproducible. Controlled concurrent read-budget rejection/release/retry behavior is also reproducible using buffer implementations from before the current branch's last six commits.
The fallback can still close a small current TsFileProcessor through DirectFlushPolicy. Avoiding duplicate submissions does not eliminate that small-file risk. A same-workload old/new baseline covering flush counts, memtable/file-size distributions, write progress, and RPC memory pressure is still required. This change does not claim that all workload read-allocation failures or small-file amplification have been eliminated.
Validation
At ab56b84:
mvn -pl iotdb-client/service-rpc verify: 108 English tests passed; dependency analysis, forbidden-API checks, and RAT passed.AutoResizingBufferTest: 19 tests passed.SystemInfoTestandTsFileProcessorTest: 10 + 26 tests passed.Regression coverage includes repeated large frames beyond the shrink count, unfinished frames written in small fragments, frame integrity, small-frame shrink count/cooldown and minimum capacity, shared budgets between connections, required-capacity fallback, failed-write rollback, close/release accounting, duplicate flush scheduling, first-tablet progress, and hard-limit rejection.
This PR has: