GGraziadei commented on PR #8787: URL: https://github.com/apache/storm/pull/8787#issuecomment-4692172125
Hi @Gowtham-Gowts, I went through the change carefully and also exercised it on a `LocalCluster`. My conclusion is that, as currntly wired, the feature does not change the tuple fate: a tuple making continuous progress is still failed at the wall-clock `topology.message.timeout.secs`. I think the design needs one more piece to actually work, and the scope should be stated more narrowly. Message timeouts are not enforced by the acker's pending map, they are enforced on the **spout** side. - `SpoutExecutor` holds its own `RotatingMap pending` (**2 buckets**, this foundamental) - The acker's pending map is created with **no callback**; when it evicts a tree it simply drops the XOR state to free memory. It never fails and never signals the spout. So re-inserting an entry into the acker's own map has no effect on whether the spout fails the tuple. The spout independently times out at `message.timeout.secs` and replays. The only mechanism that extends the spout's timer is `ACKER_RESET_TIMEOUT_STREAM_ID`. This PR never emits it. A second, structural reason the rescue can't bite today: both maps are ticked at `message.timeout.secs` (`StormCommon.java` L267 for the acker, L282 for the spout), but **the acker has 3 buckets, and the spout has 2**: The spout always fails before the acker would ever evict. The acker's extra bucket is already the safety margin that guarantees the acker doesn't forget a tree, the spout still tracks, so the rescue extends retention of trees the spout has, by then, already abandoned. ### Original STORM-2359 design https://issues.apache.org/jira/browse/STORM-2359 (there are lots of attachments) This is worth aligning with, since the issue history covers exactly this ground. In the 2017/2019 discussion, @srdo prototyped "reinserting tuples in the acker's pending upon acks" (this PR's rescue) and reported no throughput regression, so the primitive is sound. But the full design moved the timeout **into the acker** and added a **periodic spout acker sync** (anchor-id diff) so the spout could still detect loss; the acker-internal reset alone was understood not to reach the spout's pending. The work was abandoned due to performance (critical-path tracking, memory) and the sync-protocol complexity, i.e. precisely the parts this PR omits. @rzo1, this looks like an architectural block rather than a bug fix. The rescue mechanism can't reach the spout's pending map as currently wired. Please let me know your thoughts on how to align this with the broader design or the previous STORM-2359 findings/discussion. -- 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]
