kafka
kafka copied to clipboard
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
ExactlyOnceWorkerSourceTaskTest
andWorkerSourceTaskTest
suites, 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 invokeget
on the resultingFuture
on the main testing thread, which caused any uncaught errors to fail the test - With the
DistributedHerderTest
suite, 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.