L1nq0 commented on PR #9076:
URL: https://github.com/apache/storm/pull/9076#issuecomment-5563710124
@reiabreu Thanks for this review, and especially for working through the two
interaction cases I had not pushed to the end, the allocation path and the
at-least-once spout behavior. Both are now stated in the description as
limitations rather than left implicit.
The large-positive-length OOM: agreed, and agreed that catching Error is not
an answer. The description now carries it as a limitation, unchanged from
master. The bounding work, kryo input-side limits and the maxarray limit the
fallback bridge gets in #9075, is where the actual fix belongs; I have it on
the follow-up list rather than in this PR.
The spout interaction: agreed with the mechanism as you describe it, and the
description now states it. One addition worth making explicit: on master the
same deterministically undecodable message does not stall the partition
quietly. It kills the worker, the supervisor restarts it, the spout re-emits
after the tuple times out, and it kills it again, a crash loop that also never
commits the offset. So this PR trades a loud crash loop for a quiet stall,
which is the same trade it makes everywhere else, and the real fix, an eventual
permanent drop or dead-letter so the offset can advance, is the second
follow-up I want to file.
The generic-type concern and the TupleDeserializationException sketch: I
agree with the concern, and I want to push back on one boundary in the sketch.
In kryo 5.6.2, when registration is required and a name reference resolves
to a class with no registration, the IllegalArgumentException ("Class is not
registered: ...") is thrown directly by Kryo.getRegistration(Class), not
wrapped in a KryoException. DefaultClassResolver wraps only the
unknown-class-id and name-lookup failures. That raw IllegalArgumentException is
exactly what the 27-byte frame in the description produces on the live cluster
(Storm 3.x requires registration unless the java serialization fallback is
enabled), and it is the vector #9074 was filed on. With the sketch leaving
IllegalArgumentException unwrapped by design, that frame goes back to killing
the worker, which reopens the remote worker-kill this PR closes and also
undercuts the compose-with-#9075 ordering rzo1 described, since a payload the
serial filter rejects with an InvalidClassException needs the drop path to
survive.
So the trade I would rather take: keep IllegalArgumentException tolerated in
this PR, accept that a user serializer bug that raises one of the generic types
is dropped and counted rather than loud, and keep the signal that is already
built in, the WARN at 1000 consecutive failures and the always-present counter.
Then do the exception-type redesign properly in a follow-up issue where the
wrap boundary is the actual design question. Your sketch's structural
validation and single intentional exception type are the right shape, and the
problem you flag, that kryo.deserializeFrom runs the framework decode and user
serializers with nothing between them, is exactly why I do not want to rush
that boundary inside this review cycle. I will file that issue and link it
here; it can also carry a strict mode that turns drops back into fatal errors
if there is appetite for such a knob.
The minor points are both accepted and pushed. The try in recv() now covers
des.deserialize only, so updateMetrics and the batch add run outside it and a
post-decode failure propagates without being counted;
testPostDecodeFailurePropagatesAndIsNotCounted pins this by throwing a
tolerated-type exception from updateMetrics and asserting the exception escapes
with the failure counter at zero. I have also rewritten the description to
match the code: 8 types with no NullPointerException, the rate-limited logging,
and the top-level deserializationFailures key.
The full storm-client suite is green locally (662 tests) with no checkstyle
findings.
--
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]
