Hi, Let's get back to this after summer break. Thanks Antony to offering help, it might be needed.
I modified the KIP based on the feedback to be a mixture of variations 4 and 5. In TestInputTopic I removed deprecation from one variation with long timestamp and removed totally one version without key. The existing test code with it can be easily migrated to use remaining method adding null key. In TestRecord I removed constructors with Long timestamp from the public interface. You can migrate existing code with Long timestamp constructors to use constructors with ProducerRecord or ConsumerRecord. There is still Long timestamp(); method like in ProducerRecord / ConsumerRecord. Is this acceptable alternative? What else is needed to conclude the discussion phase and get to voting? Regards, Jukka to 5. syysk. 2019 klo 17.17 Antony Stubbs (ant...@confluent.io) kirjoitti: > Hi Jukka! I just came across your work - it looks great! I was taking a > stab at improving the existing API, but yours already looks great and just > about complete! Are you planning on continuing your work and submitting a > PR? If you want some help, I'd be happy to jump in. > > Regards, > Antony. > > On Thu, Aug 1, 2019 at 3:42 PM Bill Bejeck <bbej...@gmail.com> wrote: > > > Hi Jukka, > > > > I also think 3, 4, and 5 are all good options. > > > > My personal preference is 4, but I also wouldn't mind going with 5 if > that > > is what others want to do. > > > > Thanks, > > Bill > > > > On Tue, Jul 30, 2019 at 9:31 AM John Roesler <j...@confluent.io> wrote: > > > > > Hey Jukka, > > > > > > Sorry for the delay. > > > > > > For what it's worth, I think 3, 4, and 5 are all good options. I guess > my > > > own preference is 5. > > > > > > It seems like the migration pain is a one-time concern vs. having more > > > maintainable code for years thereafter. > > > > > > Thanks, > > > -John > > > > > > > > > > > > On Tue, Jul 2, 2019 at 4:03 AM Jukka Karvanen < > > jukka.karva...@jukinimi.com > > > > > > > wrote: > > > > > > > Hi Matthias, > > > > > > > > Generally I think using Instant and Duration make the test more > > readable > > > > and that's why I modified KIP based on your suggestion. > > > > Now a lot of code use time with long or Long and that make the change > > > more > > > > complicated. > > > > > > > > What I tried to say about the migration is the lines without > timestamp > > or > > > > if long timestamp is supported can be migrated mainly with search & > > > > recplace: > > > > > > > > > > > > > > > > > > testDriver.pipeInput(recordFactory.create(WordCountLambdaExample.inputTopic, > > > > nullKey, "Hello", 1L)); > > > > > > > > -> > > > > > > > > *inputTopic*.pipeInput(nullKey, "Hello", 1L); > > > > > > > > If long is not supported as timestamp, the same is not so easy: > > > > > > > > > > > > > > > > > > testDriver.pipeInput(recordFactory.create(WordCountLambdaExample.inputTopic, > > > > nullKey, "Hello", 1L)); > > > > > > > > -> > > > > > > > > *inputTopic1*.pipeInput(nullKey, "Hello", Instant.ofEpochMilli(1L)); > > > > > > > > Also if you need to convert arbitrary long timestamps to proper time > > > > Instants, it require you need to understand the logic of the test. So > > > > mechanical search & replace is not possible. > > > > > > > > > > > > I see there are several alternatives for long vs Instant / Duration: > > > > > > > > 1. All times as long/Long like in this version: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=119550011 > > > > > > > > (startTimestampMs, autoAdvanceMs as parameter of createInputTopic > > > > instead of configureTiming) > > > > > > > > 2. Auto timestamping configured with Instant and Duration, pipeInput > > > > and TestRecord with long: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=120722523 > > > > > > > > > > > > 3. (CURRENT) Auto timestamping configured with Instant and Duration, > > > > pipeInput and TestRecord with Instant, version with long deprecated: > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-470%3A+TopologyTestDriver+test+input+and+output+usability+improvements > > > > > > > > > > > > 4. Auto timestamping configured with Instant and Duration, pipeInput > > > > and TestRecord with Instant and long parallel (with long not > > > > deprecated): > > > > > > > > 5. Auto timestamping configured with Instant and Duration, pipeInput > > > > and TestRecord with Instant only > > > > > > > > I hope to get feedback. > > > > > > > > My own preference currently is alternative 3. or 4. > > > > > > > > > > > > If somebody want to test, there is a implementation of this version > > > > available in Github: > > > > > > > > https://github.com/jukkakarvanen/kafka-streams-test-topics > > > > > > > > which can be used directly from public Maven repository: > > > > > > > > <dependency> > > > > <groupId>com.github.jukkakarvanen</groupId> > > > > <artifactId>kafka-streams-test-topics</artifactId> > > > > <version>0.0.1-beta3</version> > > > > <scope>test</scope> > > > > </dependency> > > > > > > > > Also is this approach in KIP-470 preferred over KIP-456, so can we > > close > > > > it: > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-456%3A+Helper+classes+to+make+it+simpler+to+write+test+logic+with+TopologyTestDriver > > > > > > > > Jukka > > > > > > > > . > > > > > > > > > > > > pe 28. kesäk. 2019 klo 1.10 Matthias J. Sax (matth...@confluent.io) > > > > kirjoitti: > > > > > > > > > Thanks Jukka! > > > > > > > > > > The idea to use `Instant/Duration` was a proposal. If we think it's > > not > > > > > a good one, we could still stay with `long`. Because > `ProducerRecord` > > > > > and `ConsumerRecord` are both based on `long,` it might make sense > to > > > > > keep `long`? > > > > > > > > > > > The result of converting millis to Instant directly generates > > > > > >> rather non readable test code and changing from long to Instant > > > > > correctly > > > > > >> require understand what is the case it is testing. > > > > > > > > > > This might be a good indicator the using `Instant/Duration` might > not > > > be > > > > > a good idea. > > > > > > > > > > Would be nice to get feedback from others. > > > > > > > > > > About adding new methods that we deprecate immediately: I don't > think > > > we > > > > > should do this. IMHO, there are two kind of users, one that > > immediately > > > > > rewrite their code to move off deprecated methods. Those won't use > > the > > > > > new+deprecated ones anyway. Other uses migrate their code slowly > and > > > > > would just not rewrite their code at all, and thus also not use the > > > > > new+deprecated methods. > > > > > > > > > > > Checking my own tests I was able to migrate the most of my code > > with > > > > > > search&replace without further thinking about the logic to this > new > > > > > > approach. The result of converting millis to Instant directly > > > generates > > > > > > rather non readable test code and changing from long to Instant > > > > correctly > > > > > > require understand what is the case it is testing. > > > > > > > > > > Not sure if I can follow here. You first say, you could easily > > migrate > > > > > your code, but than you say it was not easily possible? Can you > > clarify > > > > > your experience upgrading your test code? > > > > > > > > > > > > > > > -Matthias > > > > > > > > > > > > > > > On 6/27/19 12:21 AM, Jukka Karvanen wrote: > > > > > > Hi, > > > > > > > > > > > >>> (4) Should we switch from `long` for timestamps to `Instant` > and > > > > > > `Duration` ? > > > > > >> This version startTimestamp is Instant and autoAdvance Duration > in > > > > > > Initialization and with manual configured collection pipe > methods. > > > > > >> Now timestamp of TestRecord is still Long and similarly single > > > record > > > > > > pipeInput still has long as parameter. > > > > > >> Should these also converted to to Instant type? > > > > > >> Should there be both long and Instant parallel? > > > > > >> I expect there are existing test codebase which would be easier > to > > > > > migrate > > > > > > if long could be still used. > > > > > > Now added Instant version to TestRecord and pipeInput method. > > > > > > > > > > > > Checking my own tests I was able to migrate the most of my code > > with > > > > > > search&replace without further thinking about the logic to this > new > > > > > > approach. The result of converting millis to Instant directly > > > generates > > > > > > rather non readable test code and changing from long to Instant > > > > correctly > > > > > > require understand what is the case it is testing. > > > > > > > > > > > > That is why version with long left still as deprecated for easier > > > > > migration > > > > > > for existing test. > > > > > > Also TopologyTestDriver constructor and advanceWallClockTime > > method > > > > > > modified with same approach. > > > > > > > > > > > > Jukka > > > > > > > > > > > > > > > > > > ma 24. kesäk. 2019 klo 16.47 Bill Bejeck (bbej...@gmail.com) > > > > kirjoitti: > > > > > > > > > > > >> Hi Jukka > > > > > >> > > > > > >>> These topic objects are only interfacing TopologyTestDriver, > not > > > > > >> affecting > > > > > >>> the internal functionality of it. In my plan the internal data > > > > > structures > > > > > >>> are using those Producer/ConsumerRecords as earlier. That way I > > > don't > > > > > see > > > > > >>> how those could be affected. > > > > > >> > > > > > >> I mistakenly thought the KIP was proposing to completely remove > > > > > >> Producer/ConsumerRecords in favor of TestRecord. But taking > > another > > > > > quick > > > > > >> look I can see the plan for using the TestRecord objects. > Thanks > > > for > > > > > the > > > > > >> clarification. > > > > > >> > > > > > >> -Bill > > > > > >> > > > > > >> On Sat, Jun 22, 2019 at 2:26 AM Jukka Karvanen < > > > > > >> jukka.karva...@jukinimi.com> > > > > > >> wrote: > > > > > >> > > > > > >>> Hi All, > > > > > >>> Hi Matthias, > > > > > >>> > > > > > >>>> (1) It's a little confusing that you list all method > (existing, > > > > > proposed > > > > > >>>> to deprecate, and new one) of `TopologyTestDriver` in the KIP. > > > Maybe > > > > > >>>> only list the ones you propose to deprecate and the new ones > you > > > > want > > > > > to > > > > > >>>> add? > > > > > >>> > > > > > >>> Ok. Unmodified methods removed. > > > > > >>> > > > > > >>>> (2) `TopologyTestDriver#createInputTopic`: might it be worth > to > > > add > > > > > >>>> overload to initialize the timetamp and auto-advance feature > > > > directly? > > > > > >>>> Otherwise, uses always need to call `configureTiming` as an > > extra > > > > > call? > > > > > >>> > > > > > >>> Added with Instant and Duration parameters. > > > > > >>> > > > > > >>>> (3) `TestInputTopic#configureTiming()`: maybe rename to > > > > > >>> `reconfigureTiming()` ? > > > > > >>> > > > > > >>> I removed this method when we have now initialization in > > > constructor. > > > > > >>> You can recreate TestInputTopic if needing to reconfigure > timing. > > > > > >>> > > > > > >>> > > > > > >>>> (4) Should we switch from `long` for timestamps to `Instant` > and > > > > > >>> `Duration` ? > > > > > >>> This version startTimestamp is Instant and autoAdvance Duration > > in > > > > > >>> Initialization and with manual configured collection pipe > > methods. > > > > > >>> Now timestamp of TestRecord is still Long and similarly single > > > record > > > > > >>> pipeInput still has long as parameter. > > > > > >>> Should these also converted to to Instant type? > > > > > >>> Should there be both long and Instant parallel? > > > > > >>> I expect there are existing test codebase which would be easier > > to > > > > > >> migrate > > > > > >>> if long could be still used. > > > > > >>> > > > > > >>> Also should advanceWallClockTime in TopologyTestDriver > > changed(or > > > > > added > > > > > >>> alternative) for Duration parameter. > > > > > >>> > > > > > >>> > > > > > >>>> (5) Why do we have redundant getters? Or set with `getX()` and > > one > > > > > >>> set without `get`-prefix? > > > > > >>> > > > > > >>> The methods without get-prefix are for compatibility with > > > > > >> ProducerRecord / > > > > > >>> ConsumerRecord and I expect would make migration to TestRecord > > > > easier. > > > > > >>> Standard getters in TestRecord enable writing test ignoring > > > selected > > > > > >> fields > > > > > >>> with hamcrest like this: > > > > > >>> > > > > > >>> assertThat(outputTopic.readRecord(), allOf( > > > > > >>> hasProperty("key", equalTo(1L)), > > > > > >>> hasProperty("value", equalTo("Hello")), > > > > > >>> hasProperty("headers", equalTo(headers)))); > > > > > >>> > > > > > >>> > > > > > >>> That's why I have currently both methods. > > > > > >>> > > > > > >>> Jukka > > > > > >>> > > > > > >>> > > > > > >>> pe 21. kesäk. 2019 klo 22.20 Matthias J. Sax ( > > > matth...@confluent.io) > > > > > >>> kirjoitti: > > > > > >>> > > > > > >>>> Thanks for the KIP. The idea to add InputTopic and OutputTopic > > > > > >>>> abstractions is really neat! > > > > > >>>> > > > > > >>>> > > > > > >>>> Couple of minor comment: > > > > > >>>> > > > > > >>>> (1) It's a little confusing that you list all method > (existing, > > > > > >> proposed > > > > > >>>> to deprecate, and new one) of `TopologyTestDriver` in the KIP. > > > Maybe > > > > > >>>> only list the ones you propose to deprecate and the new ones > you > > > > want > > > > > >> to > > > > > >>>> add? > > > > > >>>> > > > > > >>>> (Or mark all existing methods clearly -- atm, I need to got > back > > > to > > > > > the > > > > > >>>> code to read the KIP and to extract what changes are > proposed). > > > > > >>>> > > > > > >>>> > > > > > >>>> (2) `TopologyTestDriver#createInputTopic`: might it be worth > to > > > add > > > > > >>>> overload to initialize the timetamp and auto-advance feature > > > > directly? > > > > > >>>> Otherwise, uses always need to call `configureTiming` as an > > extra > > > > > call? > > > > > >>>> > > > > > >>>> > > > > > >>>> (3) `TestInputTopic#configureTiming()`: maybe rename to > > > > > >>>> `reconfigureTiming()` ? > > > > > >>>> > > > > > >>>> > > > > > >>>> (4) Should we switch from `long` for timestamps to `Instant` > and > > > > > >>>> `Duration` ? > > > > > >>>> > > > > > >>>> > > > > > >>>> (5) Why do we have redundant getters? Or set with `getX()` and > > one > > > > set > > > > > >>>> without `get`-prefix? > > > > > >>>> > > > > > >>>> > > > > > >>>> > > > > > >>>> -Matthias > > > > > >>>> > > > > > >>>> > > > > > >>>> > > > > > >>>> > > > > > >>>> On 6/21/19 10:57 AM, Bill Bejeck wrote: > > > > > >>>>> Jukka, > > > > > >>>>> > > > > > >>>>> Thanks for the KIP. I like the changes overall. > > > > > >>>>> One thing I wanted to confirm, and this may be me being > > paranoid, > > > > but > > > > > >>>> will > > > > > >>>>> the changes for input/output topic affect how the > > > > TopologyTestDriver > > > > > >>>> works > > > > > >>>>> with internal topics when there are sub-topologies created? > > > > > >>>>> > > > > > >>>>> On Fri, Jun 21, 2019 at 12:05 PM Guozhang Wang < > > > wangg...@gmail.com > > > > > > > > > > >>>> wrote: > > > > > >>>>> > > > > > >>>>>> 1) Got it, could you list this class along with all its > > > functions > > > > in > > > > > >>> the > > > > > >>>>>> proposed public APIs as well? > > > > > >>>>>> > > > > > >>>>>> 2) Ack, thanks! > > > > > >>>>>> > > > > > >>>>>> On Thu, Jun 20, 2019 at 11:27 PM Jukka Karvanen < > > > > > >>>>>> jukka.karva...@jukinimi.com> > > > > > >>>>>> wrote: > > > > > >>>>>> > > > > > >>>>>>> Hi Guozhang, > > > > > >>>>>>> > > > > > >>>>>>> 1) This TestRecord is new class in my proposal. So it is a > > > > > >> simplified > > > > > >>>>>>> version of ProducerRecord and ConsumerRecord containing > only > > > the > > > > > >>> fields > > > > > >>>>>>> needed to test record content. > > > > > >>>>>>> > > > > > >>>>>>> 2) > > > > > >>>>>>> public final <K, V> TestInputTopic<K, V> > > createInputTopic(final > > > > > >>> String > > > > > >>>>>>> topicName, final Serde<K> keySerde, final Serde<V> > > valueSerde); > > > > > >>>>>>> public final <K, V> TestOutputTopic<K, V> > > > createOutputTopic(final > > > > > >>>> String > > > > > >>>>>>> topicName, final Serde<K> keySerde, final Serde<V> > > valueSerde); > > > > > >>>>>>> The purpose is to create separate object for each input and > > > > output > > > > > >>>> topic > > > > > >>>>>>> you are using. The topic name is given to > > > createInput/OutputTopic > > > > > >>> when > > > > > >>>>>>> initialize topic object. > > > > > >>>>>>> > > > > > >>>>>>> For example: > > > > > >>>>>>> > > > > > >>>>>>> final TestInputTopic<Long, String> inputTopic1 = > > > > > >>>>>>> testDriver.createInputTopic( INPUT_TOPIC, longSerde, > > > > stringSerde); > > > > > >>>>>>> final TestInputTopic<Long, String> inputTopic2 = > > > > > >>>>>>> testDriver.createInputTopic( INPUT_TOPIC_MAP, longSerde, > > > > > >>> stringSerde); > > > > > >>>>>>> final TestOutputTopic<Long, String> outputTopic1 = > > > > > >>>>>>> testDriver.createOutputTopic(OUTPUT_TOPIC, longSerde, > > > > stringSerde); > > > > > >>>>>>> final TestOutputTopic<String, Long> outputTopic2 = > > > > > >>>>>>> testDriver.createOutputTopic(OUTPUT_TOPIC_MAP, stringSerde, > > > > > >>>>>>> longSerde); > > > > > >>>>>>> inputTopic1.pipeInput(1L, "Hello"); > > > > > >>>>>>> assertThat(outputTopic1.readKeyValue(), equalTo(new > > > > KeyValue<>(1L, > > > > > >>>>>>> "Hello"))); > > > > > >>>>>>> assertThat(outputTopic2.readKeyValue(), equalTo(new > > > > > >>> KeyValue<>("Hello", > > > > > >>>>>>> 1L))); > > > > > >>>>>>> inputTopic2.pipeInput(1L, "Hello"); > > > > > >>>>>>> > > > > > >>>>>>> > > > > > >>>>>>> Jukka > > > > > >>>>>>> > > > > > >>>>>>> to 20. kesäk. 2019 klo 23.52 Guozhang Wang ( > > wangg...@gmail.com > > > ) > > > > > >>>>>> kirjoitti: > > > > > >>>>>>> > > > > > >>>>>>>> Hello Jukka, > > > > > >>>>>>>> > > > > > >>>>>>>> Thanks for writing the KIP, I have a couple of quick > > > questions: > > > > > >>>>>>>> > > > > > >>>>>>>> 1) Is "TestRecord" an existing class that you propose to > > > > > >> piggy-back > > > > > >>>> on? > > > > > >>>>>>>> Right now we have a scala TestRecord case class but I > doubt > > > that > > > > > >> was > > > > > >>>>>> your > > > > > >>>>>>>> proposal, or are you proposing to add a new Java class? > > > > > >>>>>>>> > > > > > >>>>>>>> 2) Would the new API only allow a single input / output > > topic > > > > with > > > > > >>>>>>>> `createInput/OutputTopic`? If not, when we call pipeInput > > how > > > to > > > > > >>>>>>> determine > > > > > >>>>>>>> which topic this record should be pipe to? > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> Guozhang > > > > > >>>>>>>> > > > > > >>>>>>>> On Mon, Jun 17, 2019 at 1:34 PM John Roesler < > > > j...@confluent.io > > > > > > > > > > >>>>>> wrote: > > > > > >>>>>>>> > > > > > >>>>>>>>> Woah, I wasn't aware of that Hamcrest test style. > Awesome! > > > > > >>>>>>>>> > > > > > >>>>>>>>> Thanks for the updates. I look forward to hearing what > > others > > > > > >>> think. > > > > > >>>>>>>>> > > > > > >>>>>>>>> -John > > > > > >>>>>>>>> > > > > > >>>>>>>>> On Mon, Jun 17, 2019 at 4:12 AM Jukka Karvanen > > > > > >>>>>>>>> <jukka.karva...@jukinimi.com> wrote: > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> Wiki page updated: > > > > > >>>>>>>>>> > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-470%3A+TopologyTestDriver+test+input+and+output+usability+improvements > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> ClientRecord removed and replaced with TestRecord in > > method > > > > > >> calls. > > > > > >>>>>>>>>> TestRecordFactory removed (time tracking functionality > to > > be > > > > > >>>>>> included > > > > > >>>>>>>> to > > > > > >>>>>>>>>> TestInputTopic) > > > > > >>>>>>>>>> OutputVerifier deprecated > > > > > >>>>>>>>>> TestRecord topic removed and getters added > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> Getters in TestRecord enable writing test ignoring > > selected > > > > > >> fields > > > > > >>>>>>> with > > > > > >>>>>>>>>> hamcrest like this: > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> assertThat(outputTopic.readRecord(), allOf( > > > > > >>>>>>>>>> hasProperty("key", equalTo(1L)), > > > > > >>>>>>>>>> hasProperty("value", equalTo("Hello")), > > > > > >>>>>>>>>> hasProperty("headers", equalTo(headers)))); > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> Jukka > > > > > >>>>>>>>>> > > > > > >>>>>>>>>> la 15. kesäk. 2019 klo 1.10 John Roesler ( > > j...@confluent.io > > > ) > > > > > >>>>>>>> kirjoitti: > > > > > >>>>>>>>>> > > > > > >>>>>>>>>>> Sounds good. Thanks as always for considering my > > feedback! > > > > > >>>>>>>>>>> -John > > > > > >>>>>>>>>>> > > > > > >>>>>>>>>>> On Fri, Jun 14, 2019 at 12:12 PM Jukka Karvanen > > > > > >>>>>>>>>>> <jukka.karva...@jukinimi.com> wrote: > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> Ok, I will modify KIP Public Interface in a wiki based > > on > > > > the > > > > > >>>>>>>>> feedback. > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> TestRecordFactory / ConsumerRecordFactory was used by > > > > > >>>>>>>> TestInputTopic > > > > > >>>>>>>>> with > > > > > >>>>>>>>>>>> the version I had with KIP456, but maybe I can merge > > That > > > > > >>>>>>>>> functionality > > > > > >>>>>>>>>>> to > > > > > >>>>>>>>>>>> InputTopic or TestRecordFactory can kept non public > > > maybe > > > > > >>>>>>> moving > > > > > >>>>>>>>> it to > > > > > >>>>>>>>>>>> internals package. > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> I will make the proposal with a slim down interface. > > > > > >>>>>>>>>>>> I don't want to go to so slim as you proposed with > only > > > > > >>>>>>> TestRecord > > > > > >>>>>>>> or > > > > > >>>>>>>>>>>> List<TestRecord>, because you then still end up doing > > > helper > > > > > >>>>>>>> methods > > > > > >>>>>>>>> to > > > > > >>>>>>>>>>>> construct List of TestRecord. > > > > > >>>>>>>>>>>> The list of values is easier to write and clearer to > > read > > > > than > > > > > >>>>>> if > > > > > >>>>>>>> you > > > > > >>>>>>>>>>> need > > > > > >>>>>>>>>>>> to contruct list of TestRecords. > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> For example: > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> final List<String> inputValues = Arrays.asList( > > > > > >>>>>>>>>>>> "Apache Kafka Streams Example", > > > > > >>>>>>>>>>>> "Using Kafka Streams Test Utils", > > > > > >>>>>>>>>>>> "Reading and Writing Kafka Topic" > > > > > >>>>>>>>>>>> ); > > > > > >>>>>>>>>>>> inputTopic.pipeValueList(inputValues); > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> Let's check after the next iteration is it still worth > > > > > >> reducing > > > > > >>>>>>> the > > > > > >>>>>>>>>>> methods. > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> Jukka > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>> pe 14. kesäk. 2019 klo 18.27 John Roesler ( > > > > j...@confluent.io) > > > > > >>>>>>>>> kirjoitti: > > > > > >>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Thanks, Jukka, > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Ok, I buy this reasoning. > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Just to echo what I think I read, you would just drop > > > > > >>>>>>>> ClientRecord > > > > > >>>>>>>>>>>>> from the proposal, and TestRecord would stand on its > > own, > > > > > >>>>>> with > > > > > >>>>>>>> the > > > > > >>>>>>>>>>>>> same methods and properties you proposed, and the > > "input > > > > > >>>>>> topic" > > > > > >>>>>>>>> would > > > > > >>>>>>>>>>>>> accept TestRecord, and the "output topic" would > produce > > > > > >>>>>>>> TestRecord? > > > > > >>>>>>>>>>>>> Further, the "input and output topic" classes would > > > > > >>>>>> internally > > > > > >>>>>>>>> handle > > > > > >>>>>>>>>>>>> the conversion to and from ConsumerRecord and > > > > ProducerRecord > > > > > >>>>>> to > > > > > >>>>>>>>> pass > > > > > >>>>>>>>>>>>> to and from the TopologyTestDriver? > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> This seems good to me. > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Since the object coming out of the "output topic" is > > much > > > > > >>>>>> more > > > > > >>>>>>>>>>>>> ergonomic, I suspect we won't need the OutputVerifier > > at > > > > all. > > > > > >>>>>>> It > > > > > >>>>>>>>> was > > > > > >>>>>>>>>>>>> mostly needed because of all the extra junk in > > > > ProducerRecord > > > > > >>>>>>> you > > > > > >>>>>>>>> need > > > > > >>>>>>>>>>>>> to ignore. It seems better to just deprecate it. If > in > > > the > > > > > >>>>>>> future > > > > > >>>>>>>>> it > > > > > >>>>>>>>>>>>> turns out there is some actual use case for a > verifier, > > > we > > > > > >>>>>> can > > > > > >>>>>>>>> have a > > > > > >>>>>>>>>>>>> very small KIP to add one. But reading your > response, I > > > > > >>>>>> suspect > > > > > >>>>>>>>> that > > > > > >>>>>>>>>>>>> existing test verification libraries would be > > sufficient > > > on > > > > > >>>>>>> their > > > > > >>>>>>>>> own. > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Similarly, it seems like we can shrink the total > > > interface > > > > by > > > > > >>>>>>>>> removing > > > > > >>>>>>>>>>>>> the TestRecordFactory from the proposal. If > TestRecord > > > > > >>>>>> already > > > > > >>>>>>>>> offers > > > > > >>>>>>>>>>>>> all the constructors you'd want, then the only > benefit > > of > > > > the > > > > > >>>>>>>>> factory > > > > > >>>>>>>>>>>>> is to auto-increment the timestamps, but then again, > > the > > > > > >>>>>> "input > > > > > >>>>>>>>> topic" > > > > > >>>>>>>>>>>>> can already do that (e.g., it can do it if the record > > > > > >>>>>> timestamp > > > > > >>>>>>>> is > > > > > >>>>>>>>> not > > > > > >>>>>>>>>>>>> set). > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Likewise, if the TestRecords are easy to create, then > > we > > > > > >>>>>> don't > > > > > >>>>>>>> need > > > > > >>>>>>>>>>>>> all the redundant methods in "input topic" to pipe > > > values, > > > > or > > > > > >>>>>>>>>>>>> key/values, or key/value/timestamp, etc. We can do > with > > > > just > > > > > >>>>>>> two > > > > > >>>>>>>>>>>>> methods, one for a single TestRecord and one for a > > > > collection > > > > > >>>>>>> of > > > > > >>>>>>>>> them. > > > > > >>>>>>>>>>>>> This reduces API ambiguity and also dramatically > > > decreases > > > > > >>>>>> the > > > > > >>>>>>>>> surface > > > > > >>>>>>>>>>>>> area of the interface, which ultimately makes it much > > > > easier > > > > > >>>>>> to > > > > > >>>>>>>>> use. > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> It's best to start with the smallest interface that > > will > > > do > > > > > >>>>>> the > > > > > >>>>>>>> job > > > > > >>>>>>>>>>>>> and expand it upon request, rather than throwing in > > > > > >>>>>> everything > > > > > >>>>>>>> you > > > > > >>>>>>>>> can > > > > > >>>>>>>>>>>>> think of up front. The extra stuff would be confusing > > to > > > > > >>>>>> people > > > > > >>>>>>>>> facing > > > > > >>>>>>>>>>>>> two practically identical paths to accomplish the > same > > > > goal, > > > > > >>>>>>> and > > > > > >>>>>>>>> it's > > > > > >>>>>>>>>>>>> very difficult to slim the interface down later, > > because > > > we > > > > > >>>>>>> don't > > > > > >>>>>>>>>>>>> really know which parts are more popular (i.e., no > one > > > > > >>>>>> submits > > > > > >>>>>>>>>>>>> "feature requests" to _remove_ stuff they don't need, > > > only > > > > to > > > > > >>>>>>>> _add_ > > > > > >>>>>>>>>>>>> stuff that they need. > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> But overall, I really like the structure of this > > design. > > > > I'm > > > > > >>>>>>>> super > > > > > >>>>>>>>>>>>> excited about this KIP. > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> Thanks, > > > > > >>>>>>>>>>>>> -John > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> On Fri, Jun 14, 2019 at 2:55 AM Jukka Karvanen > > > > > >>>>>>>>>>>>> <jukka.karva...@jukinimi.com> wrote: > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> Hi, > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> I am not a fan of swapping only ProducerRecord and > > > > > >>>>>>>>> ConsumerRecord. > > > > > >>>>>>>>>>>>>> As a test writer point of view I do not want to care > > > about > > > > > >>>>>>> the > > > > > >>>>>>>>>>> difference > > > > > >>>>>>>>>>>>>> of those and > > > > > >>>>>>>>>>>>>> that way I like to have object type which can be > used > > to > > > > > >>>>>> pipe > > > > > >>>>>>>>>>> records in > > > > > >>>>>>>>>>>>>> and compare outputs. > > > > > >>>>>>>>>>>>>> That way avoid unnecessary conversions between > > > > > >>>>>> ProducerRecord > > > > > >>>>>>>> and > > > > > >>>>>>>>>>>>>> ConsumerRecord. > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> My initial assumption was that ProducerRecord and > > > > > >>>>>>>>>>> ConsumerRecord.could > > > > > >>>>>>>>>>>>>> implement the same ClientRecord > > > > > >>>>>>>>>>>>>> and that way test writer could have used either of > > > those, > > > > > >>>>>> but > > > > > >>>>>>>>> seems > > > > > >>>>>>>>>>> that > > > > > >>>>>>>>>>>>>> return type of timestamp method long vs Long is not > > > > > >>>>>>> compatible. > > > > > >>>>>>>>>>>>>> Now the main advantage of ClientRecord is no need to > > > > > >>>>>>> duplicate > > > > > >>>>>>>>>>>>>> OutputVerifier when it is modified from > ProducerRecord > > > to > > > > > >>>>>>>>>>> ClientRecord. > > > > > >>>>>>>>>>>>>> Generally is there need for OutputVerifier. Can we > > > > > >>>>>> deprecate > > > > > >>>>>>>> the > > > > > >>>>>>>>>>> existing > > > > > >>>>>>>>>>>>>> and use standard assertion libraries for new test. > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> If you use hamcrest, assert-j or any other assertion > > > > > >>>>>> library > > > > > >>>>>>>>> for the > > > > > >>>>>>>>>>>>> rest > > > > > >>>>>>>>>>>>>> of the test, why not use it with these also. > > > > > >>>>>>>>>>>>>> When we have these methods to access only needed > > fields > > > it > > > > > >>>>>> is > > > > > >>>>>>>>> easier > > > > > >>>>>>>>>>> to > > > > > >>>>>>>>>>>>>> write test like this: > > > > > >>>>>>>>>>>>>> > > assertThat(outputTopic.readValue()).isEqualTo("Hello"); > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> or > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>> > > assertThat(outputTopic.readRecord()).isEqualTo(expectedRecord); > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> Only value for new OutputVerifier is when needing to > > > > ignore > > > > > >>>>>>>> some > > > > > >>>>>>>>>>> fields > > > > > >>>>>>>>>>>>>> ClientRecord actual = outputTopic.readRecord(); > > > > > >>>>>>>>>>>>>> assertThat(actual.value()).isEqualTo("Hello"); > > > > > >>>>>>>>>>>>>> assertThat(actual.key()).isEqualTo(expectedKey); > > > > > >>>>>>>>>>>>>> > > > > > >>>>>> assertThat(actual.timestamp()).isEqualTo(expectedTimestamp); > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> So if want to leave client package untouched, I > would > > > > > >>>>>> modify > > > > > >>>>>>>> the > > > > > >>>>>>>>>>> methods > > > > > >>>>>>>>>>>>>> with ClientRecord now in InputTopic and OutputTopic > to > > > > pass > > > > > >>>>>>> in > > > > > >>>>>>>>> and > > > > > >>>>>>>>>>> out > > > > > >>>>>>>>>>>>> this > > > > > >>>>>>>>>>>>>> TestRecord instead. > > > > > >>>>>>>>>>>>>> In that case there would be possibility to add > methods > > > to > > > > > >>>>>>>>> TestRecord > > > > > >>>>>>>>>>> to > > > > > >>>>>>>>>>>>>> help ignore some fields in assertions like: > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>> > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > assertThat(outputTopic.readRecord().getValueTimestamp()).isEqualTo(expectedRecord.get > > > > > >>>>>>>>>>>>>> ValueTimestamp()); > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> How about this alternative? > > > > > >>>>>>>>>>>>>> If this way sounds better I will modify KIP page in > > > wiki. > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> Jukka > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>> to 13. kesäk. 2019 klo 18.30 John Roesler ( > > > > > >>>>>> j...@confluent.io > > > > > >>>>>>> ) > > > > > >>>>>>>>>>> kirjoitti: > > > > > >>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> Hey, all, maybe we can jump-start this discussion. > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> I think this approach would be very ergonomic for > > > > > >>>>>> testing, > > > > > >>>>>>>> and > > > > > >>>>>>>>>>> would > > > > > >>>>>>>>>>>>>>> help reduce boilerplate in tests. > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> The think I like most about it is that it mirrors > the > > > > > >>>>>>> mental > > > > > >>>>>>>>> model > > > > > >>>>>>>>>>>>>>> that people already have from Kafka Streams, in > which > > > you > > > > > >>>>>>>>> write to > > > > > >>>>>>>>>>> an > > > > > >>>>>>>>>>>>>>> "input topic" and then get your results from an > > "output > > > > > >>>>>>>>> topic". As > > > > > >>>>>>>>>>> a > > > > > >>>>>>>>>>>>>>> side benefit, the input and output topics in the > > > proposal > > > > > >>>>>>>> also > > > > > >>>>>>>>>>> close > > > > > >>>>>>>>>>>>>>> over the serdes, which makes it much less > boilerplate > > > for > > > > > >>>>>>>> test > > > > > >>>>>>>>>>> code. > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> If I can offer one suggestion for change, I'm not > > sure > > > > > >>>>>> I'm > > > > > >>>>>>>>> totally > > > > > >>>>>>>>>>>>>>> sold on the need for a new abstraction > "ClientRecord" > > > > > >>>>>> with > > > > > >>>>>>> an > > > > > >>>>>>>>>>>>>>> implementation for tests "TestRecord". It seems > like > > > this > > > > > >>>>>>>>>>>>>>> unnecessarily complicates the main (non-testing) > data > > > > > >>>>>>> model. > > > > > >>>>>>>> It > > > > > >>>>>>>>>>> seems > > > > > >>>>>>>>>>>>>>> like it would be sufficient, and just as ergonomic, > > to > > > > > >>>>>> have > > > > > >>>>>>>> the > > > > > >>>>>>>>>>> input > > > > > >>>>>>>>>>>>>>> topic accept ProducerRecords and the output topic > > > return > > > > > >>>>>>>>>>>>>>> ConsumerRecords. I'm open to discussion on this > > point, > > > > > >>>>>>>> though. > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> Thanks for the proposal, Jukka! > > > > > >>>>>>>>>>>>>>> -John > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> On Mon, May 20, 2019 at 7:59 AM Jukka Karvanen > > > > > >>>>>>>>>>>>>>> <jukka.karva...@jukinimi.com> wrote: > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> Hi All, > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> I would like to start the discussion on KIP-470: > > > > > >>>>>>>>>>> TopologyTestDriver > > > > > >>>>>>>>>>>>> test > > > > > >>>>>>>>>>>>>>>> input and output usability improvements: > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>> > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-470%3A+TopologyTestDriver+test+input+and+output+usability+improvements > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> This KIP is inspired by the Discussion in KIP-456: > > > > > >>>>>> Helper > > > > > >>>>>>>>>>> classes to > > > > > >>>>>>>>>>>>> make > > > > > >>>>>>>>>>>>>>>> it simpler to write test logic with > > > TopologyTestDriver: > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-456 > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>> > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > %3A+Helper+classes+to+make+it+simpler+to+write+test+logic+with+TopologyTestDriver > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> The proposal in KIP-456 > > > > > >>>>>>>>>>>>>>>> < > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>> > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-456%3A+Helper+classes+to+make+it+simpler+to+write+test+logic+with+TopologyTestDriver > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> was > > > > > >>>>>>>>>>>>>>>> to add alternate way to input and output topic, > but > > > > > >>>>>> this > > > > > >>>>>>>> KIP > > > > > >>>>>>>>>>> enhance > > > > > >>>>>>>>>>>>>>> those > > > > > >>>>>>>>>>>>>>>> classes and deprecate old functionality to make > > clear > > > > > >>>>>>>>> interface > > > > > >>>>>>>>>>> for > > > > > >>>>>>>>>>>>> test > > > > > >>>>>>>>>>>>>>>> writer to use. > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> In current KIP-470 proposal, topic objects are > > created > > > > > >>>>>>> with > > > > > >>>>>>>>>>>>> topicName and > > > > > >>>>>>>>>>>>>>>> related serders. > > > > > >>>>>>>>>>>>>>>> public final <K, V> TestInputTopic<K, V> > > > > > >>>>>>>>>>> createInputTopic(final > > > > > >>>>>>>>>>>>>>> String > > > > > >>>>>>>>>>>>>>>> topicName, final Serde<K> keySerde, final Serde<V> > > > > > >>>>>>>>> valueSerde); > > > > > >>>>>>>>>>>>>>>> public final <K, V> TestOutputTopic<K, V> > > > > > >>>>>>>>>>> createOutputTopic(final > > > > > >>>>>>>>>>>>>>> String > > > > > >>>>>>>>>>>>>>>> topicName, final Serde<K> keySerde, final Serde<V> > > > > > >>>>>>>>> valueSerde); > > > > > >>>>>>>>>>>>>>>> One thing I wondered if there way to find out > topic > > > > > >>>>>>> related > > > > > >>>>>>>>> serde > > > > > >>>>>>>>>>>>> from > > > > > >>>>>>>>>>>>>>>> TopologyTestDriver topology, it would simply > > creation > > > > > >>>>>> of > > > > > >>>>>>>>> these > > > > > >>>>>>>>>>> Topic > > > > > >>>>>>>>>>>>>>>> objects: > > > > > >>>>>>>>>>>>>>>> public final <K, V> TestInputTopic<K, V> > > > > > >>>>>>>>>>> createInputTopic(final > > > > > >>>>>>>>>>>>>>> String > > > > > >>>>>>>>>>>>>>>> topicName); > > > > > >>>>>>>>>>>>>>>> public final <K, V> TestOutputTopic<K, V> > > > > > >>>>>>>>>>> createOutputTopic(final > > > > > >>>>>>>>>>>>>>> String > > > > > >>>>>>>>>>>>>>>> topicName); > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> KIP-456 can be discarded if this KIP get accepted. > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>>>>> Best Regards, > > > > > >>>>>>>>>>>>>>>> Jukka > > > > > >>>>>>>>>>>>>>> > > > > > >>>>>>>>>>>>> > > > > > >>>>>>>>>>> > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> -- > > > > > >>>>>>>> -- Guozhang > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>>>> > > > > > >>>>>> -- > > > > > >>>>>> -- Guozhang > > > > > >>>>>> > > > > > >>>>> > > > > > >>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > Antony Stubbs > > Solutions Architect | Confluent > > +44 (0)7491 833 814 <+447491833814> > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog > <http://www.confluent.io/blog> >
