GGraziadei commented on PR #8796: URL: https://github.com/apache/storm/pull/8796#issuecomment-4817613179
Hi @reiabreu, Thanks for scanning this PR with an agent, I think it is a really good practice to always get new insights to reflect on. **Regarding point A**, I think your analysis makes total sense. At the same time, we are running into one of Java's biggest limitations, which is its memory management. I perfectly understand the highlighted issue and it is real. In other programming languages with direct memory management (like Rust or C++), for example, I remember the solution is simply to call the destructor when a weak reference no longer exists. In Rust, for instance, I remember using special smart pointers like `Arc` or `Weak` to handle these situations. All of this is not possible in Java, but browsing the documentation I found this, which I believe could be a partial solution: https://docs.oracle.com/javase/8/docs/api/java/lang/ref/WeakReference.html It certainly won't call a destructor, but I believe it will be useful at the GC level to break the reference cycle. Since this pattern was already pre-existing and requires some proper refactoring, I have opened this dedicated issue to fix it later: https://github.com/apache/storm/issues/8810 **Regarding point B**: Your reconstruction is spot on, and this behavior is entirely intentional. A full batch that the `recvQueue` cannot accept is deliberately treated as a heavy-load signal, triggering an additive increase in the effective batch size. This mechanism is safe because it is both bounded (strictly capping at `maxBatchSz`, where backpressure cleanly propagates upstream) and self-correcting (once the spike subsides, the flush-tuple timer triggers a partial flush, causing a multiplicative decrease back toward 1). It's also consistent with the blocking `flush()`, which grows on a full batch as well, it just does so *after* its retry loop drains the queue, whereas `tryFlush()` (which can't retry) adapts immediately. Same direction, same end state for `wasFull == true`. Furthermore, under active backpressure the primary driver of latency is downstream congestion and queue waiting time, not the local batch size itself, so the extra buffering is effectively free here. I've added an explanatory comment next to the `afterFlush` call in `tryFlush()` to document this intent, so it won't be mistaken for a bug during future cleanups. Thanks for the deep dive and the feedback! -- 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]
