Hi John, Thank you for your review and the feedback. In existing method TTD.close(), stateDirectory.clean() <https://github.com/apache/kafka/blob/trunk/streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java#L1193> method is getting called which is cleaning up task and global directories <https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java#L285-L291>. If RocksDB directories are not getting cleaned up in that close method, would like to hear about how we can clean them up in that method. Currently default value of state_directory is set to /tmp/kafka-streams <https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java#L605> so I am not setting it's value explicitly in proposed no argument constructor. Does the directory have to be unique in each test? If yes, then I agree that we can tackle RocksDb directories cleanup and creating unique directory tasks in separate KIP.
Thanks, Rohit On Fri, Nov 6, 2020 at 7:12 AM John Roesler <vvcep...@apache.org> wrote: > Hello Rohit, > > Thanks for picking this up! I think your KIP looks good. > > While I was doing some cleanup of our tests before, one thing I > encountered is that, while most tests don’t semantically need to specify > any configs, many tests actually do set the state directory config. They > set it specifically so that they can delete it at the end of the test. > Otherwise, the tests would leave RocksDB directories behind. > > I’m wondering if we should address this issue as part of your KIP. What > I’m thinking is this: if no state directory is specified, then we create a > new, unique temp directory and register it for cleanup when the JVM exits. > Additionally, we would set a flag and clean up the state dir when > TTD.close() is called. > > That way, TTD tests would be by default independent and tidy. > > Admittedly, this is outside the current scope of your KIP, so please feel > free to reject this idea, in which case I can file a separate ticket for > it. > > Thanks! > -John > > On Tue, Nov 3, 2020, at 18:59, Rohit Deshpande wrote: > > Hi Matthias, > > Thank you for the review and the suggestion. > > Considering at most 3 parameters to the constructor of > > TopologyTestDriver > > and topology being required parameter, we can definitely add a new > > constructor `TopologyTestDriver(Topology, Instant)` . > > Right now, I see one test where we can use this constructor: > > > https://github.com/apache/kafka/blob/trunk/streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamTransformTest.java#L80-L83 > > Also we can get rid of this method in TestDriver trait: > > > https://github.com/apache/kafka/blob/trunk/streams/streams-scala/src/test/scala/org/apache/kafka/streams/scala/utils/TestDriver.scala#L32-L35 > > which is used in multiple test classes and seems redundant. I agree with > > your suggestion. > > Thanks, > > Rohit > > > > On Tue, Nov 3, 2020 at 3:19 PM Matthias J. Sax <mj...@apache.org> wrote: > > > > > Thanks for the KIP Rohit. > > > > > > Wondering, if we should also add `TopologyTestDriver(Topology, > > > Instant)`? Not totally sure, as having too many overload could also be > > > annoying. > > > > > > > > > -Matthias > > > > > > On 11/3/20 10:02 AM, Rohit Deshpande wrote: > > > > Hello all, > > > > I have created KIP-680: TopologyTestDriver should not require a > > > Properties > > > > argument. > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-680%3A+TopologyTestDriver+should+not+require+a+Properties+argument > > > > > > > > Jira for the KIP: > > > > https://issues.apache.org/jira/browse/KAFKA-10629 > > > > > > > > If we end up making changes, they will look like this: > > > > https://github.com/apache/kafka/compare/trunk...rohitrmd:KAFKA-10629 > > > > > > > > Please have a look and let me know what you think. > > > > > > > > Thanks, > > > > Rohit > > > > > > > > > >
