Thanks Rohit! This looks good to me. As far as I’m concerned, you could start the voting thread.
-John On Wed, Nov 11, 2020, at 21:21, Rohit Deshpande wrote: > 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 > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > >
