kafka icon indicating copy to clipboard operation
kafka copied to clipboard

MINOR: Remove unused ShutdownableThread class and ineffective ThreadedTest classes

Open C0urante opened this issue 2 years ago • 2 comments

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)

C0urante avatar Jul 14 '22 23:07 C0urante

Looks great to me!

clolov avatar Jul 20 '22 08:07 clolov

Wouldn't we want to integrate this usage in more tests versus removing the classes?

ijuma avatar Aug 10 '22 13:08 ijuma

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 and WorkerSourceTaskTest suites, we were spinning up at most one additional thread per test via ExecutorService::submit, and every time we did that, we were already making sure to invoke get on the resulting Future 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 via ExecutorService::submit, but not checking the resulting Future. 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.

C0urante avatar Aug 15 '22 19:08 C0urante

Thanks for the explanation.

ijuma avatar Aug 17 '22 20:08 ijuma