kennknowles commented on pull request #13206: URL: https://github.com/apache/beam/pull/13206#issuecomment-720678222
IIRC there are some DirectRunner tests around leaking state. It doesn't quite fit with "ValidatesRunner" because `runners-core` is a utility library that is an optional implementation detail, not an externally observable behavior. A runner isn't required to use this form of state at all, or Java, etc. I know of at least one runner that does its triggering outside of Java, but indeed most of them _will_ use this indefinitely. With regard to runners-core and `ReduceFnRunner`, I agree that all the contexts and namespaces can be quite confusing. The code is quite obscure, especially considering how elementary the actual functionality. Once or twice I've tried to untangle it but failed each time. A major refactor or rewrite might actually be easier than incremental simplification. I'd want to get a clearer test suite that is truly the spec for triggers before doing that. Basically the spirit of `ReduceFnRunnerTest` but expressed as external `TestStream`-based end-to-end behavioral tests instead of heavily mocked out (hence unreliable) unit tests. This won't improve the ability to introspect intermediate state but it will make the code simpler. Meanwhile, I do think it is reasonable to add accessors to the various internal contexts and have `ReduceFnRunner` assert things about its own behavior. As long as it stays "in bounds" and only asserts things that it really controls and that cannot vary across different uses of `ReduceFnRunner`. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
