Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Guozhang Wang]

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  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 
> > 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 
> 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 
> > >> 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 

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[John Roesler]

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 
> 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  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 
> >> 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 
> >> 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.
> >> > 

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Rohit Deshpande]

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 
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  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 
>> 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 
>> 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
>> > > > 

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Rohit Deshpande]

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  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  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 
> 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
> > > > > >
> > 

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[John Roesler]

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()
> 
> method is getting called which is cleaning up task and global 
> directories
> .
> 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
> 
> 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  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  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
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Rohit Deshpande]

Hi John,
Thank you for your review and the feedback.

In existing method TTD.close(),  stateDirectory.clean()

method is getting called which is cleaning up task and global directories
.
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

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  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  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
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[John Roesler]

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  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
> > >
> >
>

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Rohit Deshpande]

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  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
> >
>

Re: [DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Matthias J. Sax]

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
> 

[DISCUSS] KIP-680: TopologyTestDriver should not require a Properties argument

[Rohit Deshpande]

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