Caideyipi commented on PR #17911: URL: https://github.com/apache/iotdb/pull/17911#issuecomment-4678160429
Thanks for adding the RPC AutoResizingBuffer memory control. I found a few issues that should be fixed before merge: 1. **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. 2. **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. 3. **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. 4. **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. 5. **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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
