rzo1 commented on PR #9154: URL: https://github.com/apache/storm/pull/9154#issuecomment-6387517790
Thanks for the thorough follow-up. All points from my earlier reviews are addressed or declined with good reasons, and the disabled path looks allocation-free to me. Two small things before merge: 1. `TopologyTracingTest` shares one `LocalCluster` across tests and closes topologies with `KILL_NOW`. A late span from one test could land in the next test's exact-count asserts, i.e. the same flake you fixed in `TupleTracerTest` (bf6cbf312). Could you use a cluster per test there as well? 2. After the rebase, the commits cited in the description (e.g. ecdfe2fb1, 59f06daf3, 42e1430cc) no longer exist on the branch. Could you update the references? We should squash on merge, since the early commits add and then remove OTel in storm-client. Optional, but worth deciding before `TupleTracer` becomes public API: `spoutEmit` gets neither the message id nor the values, so a tracer cannot continue an upstream trace through that hook later without a new default method. -- 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]
