L1nq0 commented on PR #9076: URL: https://github.com/apache/storm/pull/9076#issuecomment-5547635570
Thanks for the careful review, and for independently checking the two spots I was most concerned about: the Kryo reset on a mid-stream failure and the MessageDecoder path. Your read matches what I found, so it's good to have it confirmed. On the numbered points: 1. Log volume: agreed, that's the remaining DoS and it cuts both ways. Plan: the first few dropped messages get individual ERROR logs, then a periodic summary line carrying the running total, following the dropCount precedent in WorkerState.dropMessage. On top of that a WARN after N consecutive failures, so the genuine missing-class case surfaces as a loud signal instead of silent no-progress with a stack trace per tuple. 2. NullPointerException: agreed that tolerating it at the callback papers over a missing check, and with the broader point about user-supplied serializers: a real bug in one of those surfacing as an NPE or IAE should fail loudly, not vanish into dropped data. Taking the minimal path in this PR: NPE comes out of the tolerated set, and the task-id lookup in KryoTupleDeserializer gets an explicit check that throws a typed exception with the id in the message, which then flows through the tolerated path as a normal decode failure. The dedicated TupleDeserializationException wrapping everything the decode path can legitimately throw is the more durable shape for the broader concern; I'd rather track that as a follow-up issue than widen this PR's blast radius mid-review, but I'm happy either way. 3. Metric namespace: agreed on all three counts. The counter moves to the top level of Server.getState, it is always emitted with a stable zero rather than appearing only on failure, and docs/Metrics.md gets the line. Smaller things, all accepted: import order fixed to match the test file, the reflective write to the ThreadLocal replaced by a package-private constructor taking the deserializer, and the tolerance check rewritten as a single pass over the cause chain testing instanceof against the list. Merge order: agreed. This PR stays independent of #9075, and I'll rebase #9075 onto it after it lands. Thanks also for triggering the workflows. -- 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]
