Thanks John, I will go ahead and update the KIP with a randomized application id requirement.
On Fri, Nov 6, 2020 at 3:12 PM John Roesler <vvcep...@apache.org> wrote: > Hi Rohit, > > Ah, indeed, that was my mistake. I made a bad assumption about the code. > > Since we are already cleaning up, then I’d suggest only that we might > generate a randomized application id so that concurrent tests won’t > interfere with each other. But this is sounding like a minor implementation > note, not a concern for the KIP. > > The proposal looks good to me. > > Thanks again, > John > > On Fri, Nov 6, 2020, at 16:54, Rohit Deshpande wrote: > > 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 > > > > > > > > > > > > > > > > > > > > >
