[client] release the resource of record accumulator only when sender thread is stopped.#3102
[client] release the resource of record accumulator only when sender thread is stopped.#3102loserwang1024 wants to merge 2 commits intoapache:mainfrom
Conversation
8cb0b0d to
73a5c7b
Compare
utafrali
left a comment
There was a problem hiding this comment.
The core fix is correct: splitting close() (reject appends) from destroyResources() (free Arrow resources) and calling the latter only after all drain loops have finished cleanly resolves the 'Accounted size went negative' race. The main issues are an unused public getter that leaks internal state, missing lifecycle documentation on Sender.destroyResources() that hides a latent double-close risk, and a regression test with no assertion.
b1738fa to
107c3b4
Compare
…thread is stopped. # Conflicts: # fluss-client/src/main/java/org/apache/fluss/client/write/RecordAccumulator.java
107c3b4 to
134344c
Compare
|
@platinumhamburg ,CC |
| * <p>This method is idempotent: subsequent calls after the first are no-ops. | ||
| */ | ||
| public void destroyResources() { | ||
| if (resourcesDestroyed) { |
There was a problem hiding this comment.
volatile guarantees visibility but does not guarantee atomicity of the check + set. Two threads can both read false, pass the guard, and both enter the destruction path. Arrow's BufferAllocator.close() is not idempotent and will throw IllegalStateException on a second invocation.
The Javadoc claims "This method is idempotent: subsequent calls after the first are no-ops", but the implementation cannot honor this guarantee under concurrent access.
Suggested fix:
private final AtomicBoolean resourcesDestroyed = new AtomicBoolean(false);
public void destroyResources() {
if (!resourcesDestroyed.compareAndSet(false, true)) {
return;
}
writerBufferPool.close();
arrowWriterPool.close();
bufferAllocator.close();
chunkedFactory.close();
}
platinumhamburg
left a comment
There was a problem hiding this comment.
Thanks @loserwang1024, I left some comments.
| * <p>This method is idempotent: repeated calls are harmless because the underlying {@link | ||
| * RecordAccumulator#destroyResources()} tolerates multiple invocations. | ||
| */ | ||
| void destroyResources() { |
There was a problem hiding this comment.
Missing @VisibleForTesting
Purpose
Linked issue: close #3101
Brief change log
Tests
API and Format
Documentation