KAFKA-3607: Close KStreamTestDriver upon completing #1258
Conversation
guozhangwang
changed the title
|
@ijuma @enothereska @miguno for reviews.
Ran the branch on Jenkins for 15+ rounds, and do not see the |
| driver.close(); | ||
| } | ||
| driver = null; | ||
| } |
There was a problem hiding this comment.
This code seems to be repeated in a lot of tests. Is there a reason why we don't introduce a superclass for initialisation and clean-up of KStreamTestDriver?
There was a problem hiding this comment.
We could also make KStreamTestDriver a JUnit ExternalResource so that the shutdown is done automatically.
There was a problem hiding this comment.
ExternalResource: +1 (assuming it'll work that way)
There was a problem hiding this comment.
Edit: Ah, I think you mean that we need to pass builder to the constructor of KStreamTestDriver, which (in this case, unfortunately) is being mutated by subsequent calls such as builder.stream() or stream.branch() before we pass it to the driver constructor?
There was a problem hiding this comment.
Perhaps @dguy has an idea whether it's possible to use KStreamTestDriver as an ExternalResource even though, for constructing an instance of the former, we need to pass a test case specific parameter (here: builder aka KStreamBuilder instance) to it for each test.
There was a problem hiding this comment.
It is possible, but you'd need to make a few changes to KStreamTestDriver , i.e., everything currently in the constructor would need to be moved into a method that can be called from the tests. So you'd end up having an overloaded initiliaze method instead of the various constructors.
|
LGTM modulo @ijuma 's comments. Tests ran locally fine. |
|
@ijuma Please review again. I did not make |
| public void run() { | ||
| Utils.delete(file); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We don't need this for files, right? Just for directories (because of file.deleteOnExit)
|
@guozhangwang: Do we want to update |
|
@miguno As I mentioned above, I'd prefer to do it in a separate PR. |
|
@ijuma Could you take another look? |
| } | ||
| }) | ||
| f | ||
| org.apache.kafka.test.TestUtils.tempDirectory(parentFile.toPath, "kafka-"); |
There was a problem hiding this comment.
Would it not be better to use kafka- as the prefix by default in o.a.k.t.TestUtils.tempDirectory?
There was a problem hiding this comment.
Ack. Changing kafka to kafka-.
|
@guozhangwang Generally, it would be better not to introduce technical debt and then fix it in cases where the fix is straightforward. However, this helps with test stability, so I am OK for this to be merged before we do KAFKA-3623 if you address the two minor comments I left and you edit the PR description to mention that the duplication will be handled as part of KAFKA-3623 (no additional review is needed). |
…to be tracked in KAFKA-3623 Author: Guozhang Wang <wangguoz@gmail.com> Reviewers: Eno Thereska, Michael G. Noll, Ismael Juma Closes apache#1258 from guozhangwang/K3607
…movements.per.broker (apache#1258)
No description provided.