Caideyipi commented on PR #17911:
URL: https://github.com/apache/iotdb/pull/17911#issuecomment-4785104298
Thanks for the fixes. I rechecked the current PR head (`88cba485d24`). The
transport close paths, constructor partial-allocation cleanup paths, and the
`Utils.serializeTSStatus()` temporary-buffer leak from my previous review look
fixed now, and the new unit tests cover those cases.
I still see a few issues before this is ready:
1. **Blocking: the PR is currently conflicting with `master`.** GitHub
reports `mergeable: CONFLICTING`. Locally, `git merge-tree HEAD origin/master`
reports conflicts in:
-
`integration-test/src/test/java/org/apache/iotdb/pipe/it/dual/treemodel/auto/enhanced/IoTDBPipeIdempotentIT.java`
-
`iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/utils/Utils.java`
Please be careful when resolving `Utils.java`: it overlaps with the
`serializeTSStatus()` fix, so the rebased version should still copy the
serialized bytes and close the `AutoScalingBufferWriteTransport` in a `finally`
block.
2. **P2: the ConfigNode path still does not appear to have an actual
quota.** `ConfigNode.start()` now initializes `MemoryConfig`, but only DataNode
calls `setAutoResizingBufferMemoryControl(...)`. In `MemoryConfig`, when the
AutoResizingBuffer memory block is absent, `allocate()` returns `true`, so
ConfigNode RPC buffers still effectively use the no-op control. If this PR is
intended to control ConfigNode RPC buffers too, ConfigNode needs a real
budget/block. If the intended scope is DataNode only, please clarify that and
avoid implying that ConfigNode is controlled.
3. **P2: `AutoResizingBuffer` constructor can leak accounted quota if array
allocation fails.** The constructor calls
`AutoResizingBufferMemoryManager.allocate(initialCapacity)` and then `new
byte[initialCapacity]`. If the byte-array allocation throws, the accounted
quota is not released. The resize path already handles this with a try/catch
rollback; the constructor should do the same.
4. **P3: unrelated changes remain in the PR.** `pom.xml` still adds
`AGENTS.md` to RAT excludes, and `iotdb-client/client-go` is still changed as a
submodule pointer. Both look unrelated to RPC AutoResizingBuffer memory control
and should be removed from this PR.
I only ran lightweight local checks this time: `git diff --check
origin/master...HEAD` passed. I did not run 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]
