Hi John and Matthias, I have updated the wiki with your suggestions. https://cwiki.apache.org/confluence/display/KAFKA/KIP-680%3A+TopologyTestDriver+should+not+require+a+Properties+argument
Thanks, Rohit On Fri, Nov 6, 2020 at 3:23 PM Rohit Deshpande <rohitdesh...@gmail.com> wrote: > 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 >> > > > > > >> > > > > >> > > > >> > > >> > >> >
