reiabreu commented on PR #9075:
URL: https://github.com/apache/storm/pull/9075#issuecomment-5553324579
hey!
@rzo1 already covered all important points and it's a +1 on his review.
I did a sweep with the help of an LLM and this is what is outstanding:
1. Add maxarray (and maxdepth/maxrefs) to the sample pattern, not just
maxbytes. maxbytes only counts bytes already read, so one big array still gets
allocated before the limit kicks in. The class deny-list does nothing for this.
2. Say that this filter replaces a JVM-wide -Djdk.serialFilter, it does not
add to it. The default factory just returns the new filter, so a filter set in
worker.childopts is quietly dropped on this path. Either merge with the
existing one via ObjectInputFilter.merge(...), or document it.
3. Factory scope (rzo1 #3): fix it by documenting, not by moving the install
into SerializationFactory.getKryo(). The filter can only go where the
ObjectInputStream is opened, which is inside SerializableSerializer. That works
for the default factory because it's our code and it builds that serializer. A
custom topology.kryo.factory is a black box: SerializationFactory only gets
back a finished Kryo, with no way to reach its fallback path. So a generic
install can't cover custom factories. Best to just state in the Config javadoc
and both docs that the filter covers the default factory's fallback bridge, and
a custom factory is on its own.
4. Minor: maxbytes is per object, not per tuple (a new ObjectInputStream is
opened per value). Worth saying so in the docs.
--
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]
