You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for adding the RPC AutoResizingBuffer memory control. I found a few issues that should be fixed before merge:
P1: buffer memory accounting is not released when framed transports are closed.TElasticFramedTransport.close() currently only closes underlying (iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TElasticFramedTransport.java:123), but the newly accounted readBuffer and writeBuffer are only released by AutoScalingBuffer*Transport.close(). The compressed transport also allocates writeCompressBuffer and readCompressBuffer (TCompressedElasticFramedTransport.java:41) with no close path. This means normal connection close leaks the accounted quota and will eventually reject later connections/requests.
P1: constructor failure paths leak already allocated buffers. In TElasticFramedTransport (lines 94-99), readBuffer is allocated before writeBuffer; if the second allocation fails, the catch block wraps the exception but does not close the first buffer. TCompressedElasticFramedTransport has the same issue for the extra compression buffers. This can happen exactly when the memory quota is near exhaustion, so failed connection creation can consume quota permanently.
P1: Utils.serializeTSStatus() creates an accounted temporary buffer and never closes it.iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/utils/Utils.java:192 creates an AutoScalingBufferWriteTransport, writes into it, and returns without releasing the transport. With this PR, each status serialization leaks TEMP_BUFFER_SIZE from the AutoResizingBuffer quota. Please use a try/finally or try-with-resources-style close path; if needed, copy the returned bytes before closing.
P2: the ConfigNode RPC path appears to keep the default no-op memory control. The hook is registered only when MemoryConfig is initialized (iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/memory/MemoryConfig.java:33), but ConfigNodeRPCService also uses DeepCopyRpcTransportFactory (iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/service/thrift/ConfigNodeRPCService.java:79) and I could not find a MemoryConfig.global() / MemoryConfig.getInstance() initialization path under iotdb-core/confignode. If so, auto_resizing_buffer_memory_proportion does not control ConfigNode RPC buffers.
P3: unrelated RAT exclusion.pom.xml adds AGENTS.md to the apache-rat exclude list, but this PR does not add that file. This is unrelated to the feature and relaxes license checking, so it should be removed from this PR.
I only ran a lightweight whitespace check locally: git diff --check review-pr-17911/base...review-pr-17911/head passed. I did not run the Maven tests.
TElasticFramedTransport.close() now releases the accounted read/write buffers, and compressed transports release their extra compression buffers as well.
Constructor failure paths now close any buffers that were already allocated, covering both framed and compressed transports.
Utils.serializeTSStatus() now copies the serialized bytes and closes the temporary AutoScalingBufferWriteTransport in a finally block.
ConfigNode startup now initializes MemoryConfig, so the ConfigNode RPC path installs the AutoResizingBuffer memory-control hook.
Also added unit coverage for transport close/failure release paths in AutoResizingBufferTest.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests