Hello Rohit, I made a pass on the KIP and it looks good to me too. Please feel free to start a voting thread.
On Fri, Nov 13, 2020 at 7:36 AM John Roesler <vvcep...@apache.org> wrote: > 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 > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > -- -- Guozhang
