MINOR: Remove unused ShutdownableThread class and ineffective ThreadedTest classes
The ShutdownableThread class isn't used anywhere outside of tests, and the ThreadedTest class only works in cases where the logic that's being tested uses a ShutdownableThread. Both of these classes are removed, and the DistributedHerderTest class is updated to properly report unexpected exceptions that take place on other threads (which appears to be the original purpose of the ThreadedTest class, although it was not actually doing this anywhere).
Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
Looks great to me!
Wouldn't we want to integrate this usage in more tests versus removing the classes?
The ThreadedTest class was intended to detect uncaught errors on other threads than the main thread used for the test. In many cases (including the WorkerSinkTaskThreadedTest suite), this was unnecessary since tests that inherited from it were all single-threaded. The only exceptions are:
- With the
ExactlyOnceWorkerSourceTaskTestandWorkerSourceTaskTestsuites, we were spinning up at most one additional thread per test viaExecutorService::submit, and every time we did that, we were already making sure to invokegeton the resultingFutureon the main testing thread, which caused any uncaught errors to fail the test - With the
DistributedHerderTestsuite, we were also spinning up at most one additional thread per test viaExecutorService::submit, but not checking the resultingFuture. I've added a check in this PR
I toyed with the idea of keeping the ThreadedTest class as a reusable utility, but I don't think it'd save us a lot of trouble without also potentially tying our hands and preventing us from doing things like ensuring that a certain thread or task dispatched on a separate thread has completed at a certain point in the test.
The ShutdownableThread (and its test) are not used (effectively) anywhere in the code base and are completely safe to remove.
Thanks for the explanation.