rzo1 commented on PR #9094:
URL: https://github.com/apache/storm/pull/9094#issuecomment-5694114164
The unknown-stream check is correct and closes a real hole: a null stream
name was previously passed to `TupleImpl` and the tuple delivered anyway.
Sender and receiver build `IdDictionary` from the same topology and
`KryoTupleSerializer` would fail on the sending side before emitting an
unresolvable id, so a null on the receiving side does mean a corrupt or
mismatched frame. No concerns there.
Three things before this can go in.
1. `TupleDeserializationException extends RuntimeException`. The
unknown-task case threw `IllegalArgumentException` in 3.1.0, so anything
catching that stops matching. Please extend `IllegalArgumentException` instead.
Existing callers keep working, and the entry you added to
`TOLERATED_DESERIALIZATION_FAILURES` becomes redundant, though keeping it
explicit is fine. While you are in there, add a `(String, Throwable)`
constructor.
2. The config is undocumented.
`topology.tuple.deserialization.strict.enable` exists only in `Config.java` and
`defaults.yaml`. `docs/Serialization.md` carries the table where
`topology.tuple.compression.max.decompressed.bytes` is documented, and
`docs/Metrics.md` covers `deserializationFailures`, which you added in #9076.
The flag belongs in both.
3. The config javadoc does not say what enabling it costs. Under strict mode
a single corrupt frame from a peer kills the worker, the supervisor restarts
it, and the same frame kills it again. That restart loop is what #9076 fixed,
and users should read it in the config description rather than infer it. The
"pre-3.1.0 behavior" wording is accurate, #9076 is contained in v3.1.0, so keep
that and add the consequence.
Minor:
- `assertTrue(thrown.getMessage().contains("id 3"))` ties the test to the
exact message text. The exception type plus the component name would be enough.
- No test covers strict mode with the new exception.
`testStrictModeMakesFailuresFatal` only exercises the truncated payload path.
On your open question: `topology.tuple.deserialization.strict.enable` is
fine, it matches the existing keys.
--
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]
