rzo1 commented on PR #8653:
URL: https://github.com/apache/storm/pull/8653#issuecomment-4481114137
Overall LGTM — thanks for working through the review, the rolling-upgrade
fallback and the decompression cap are exactly what I was hoping for.
One follow-up suggestion before merge: the current
`ZstdBridgeThriftSerializationDelegateTest` verifies routing via
`MockedStatic<Utils.ZstdUtils>`, but it never feeds *real* Gzip-compressed
bytes into the bridge. That end-to-end path is the whole reason this class
exists (rolling upgrade from a Gzip-default cluster), so it's worth one real
round-trip test — no mocks, no reflection. The existing
`GzipBridgeThriftSerializationDelegateTest` is the template; mirroring it for
the Zstd bridge is ~50 lines:
```java
public class ZstdBridgeThriftSerializationDelegateRoundTripTest {
SerializationDelegate bridge;
@BeforeEach
public void setUp() {
bridge = new ZstdBridgeThriftSerializationDelegate();
bridge.prepare(Collections.emptyMap());
}
@Test
public void testDeserialize_readingFromGzip() {
GlobalStreamId id = new GlobalStreamId("first", "second");
byte[] serialized = new
GzipThriftSerializationDelegate().serialize(id);
GlobalStreamId out = bridge.deserialize(serialized,
GlobalStreamId.class);
assertEquals(id.get_componentId(), out.get_componentId());
assertEquals(id.get_streamId(), out.get_streamId());
}
@Test
public void testDeserialize_readingFromRawThrift() {
GlobalStreamId id = new GlobalStreamId("A", "B");
byte[] serialized = new ThriftSerializationDelegate().serialize(id);
GlobalStreamId out = bridge.deserialize(serialized,
GlobalStreamId.class);
assertEquals(id.get_componentId(), out.get_componentId());
assertEquals(id.get_streamId(), out.get_streamId());
}
@Test
public void testRoundTrip_throughBridge() {
GlobalStreamId id = new GlobalStreamId("x", "y");
byte[] serialized = bridge.serialize(id);
GlobalStreamId out = bridge.deserialize(serialized,
GlobalStreamId.class);
assertEquals(id.get_componentId(), out.get_componentId());
assertEquals(id.get_streamId(), out.get_streamId());
}
}
```
This catches the exact regression class your earlier `defaultDelegate = new
ThriftSerializationDelegate()` would have hit — mocks can't, since they
short-circuit `isZstd`. Not blocking on this from my side; happy to merge once
CI is green if you'd rather do it as a small follow-up PR.
--
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]
