Personally, I think it would be cleaner to just pass `Deserializer` into `InputTopic` and `Serializer` into `OutputTopic`. Keeping the interface simple is a good argument and we should not have overloads -- but it seems that picking `Serdes` instead of the more straight forward `(De)Serializer` is not "clean". One should only pass what one needs IMHO.
I also don't think that calling `#serializer()` if necessary is too much boilerplate. At the same time, if you don't have a `Serde` at hand, it would be quite some overhead to create a new Serde that wraps a (de)serializer one has at hand. Note that `ConsumerRecordFactory` also accepts `Serializer` only for the same reason. Beside this minor question, I think you can start a VOTE thread. If we close the vote before Wednesday (cf https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=125307901) we might be able to get it into 2.4 release (if we have the bandwidth to review the PR in time...) -Matthias On 9/13/19 10:08 PM, Jukka Karvanen wrote: > Hi, > > In many cases you have already serde for the stream definition. So I see > natural to use it also for the tests. > So you can write: > > public void setup() { > testDriver = new TopologyTestDriver(TestStream.getTopology(), > TestStream.getConfig()); > inputTopic = testDriver.createInputTopic(TestStream.INPUT_TOPIC, > keySerde, myClassSerde); > outputTopic = testDriver.createOutputTopic(TestStream.OUTPUT_TOPIC, k > eySerde, outClassSerde); > } > > Instead of > > public void setup() { > testDriver = new TopologyTestDriver(TestStream.getTopology(), > TestStream.getConfig()); > inputTopic = testDriver.createInputTopic(TestStream.INPUT_TOPIC, > keySerde.serializer(), myClassSerde.serializer()); > outputTopic = testDriver.createOutputTopic(TestStream.OUTPUT_TOPIC, k > eySerde.deserializer(), outClassSerde.deserializer()); > } > > In original KIP-456 I had variation for Contructors with Serde and > Serializer/Deserializer, but based on the request to keep the interface as > simple as possible only version with Serde was kept. > Jukka > > > > > > pe 13. syysk. 2019 klo 19.15 Matthias J. Sax (matth...@confluent.io) > kirjoitti: > >> Maybe one follow up question: >> >> Why do we pass `Serdes` into `createInputTopic` and createOutputTopic` >> -- seems `Serializer` (for input) and `Deserialized` (for output) should >> be sufficient? >> >> >> -Matthias >> >> On 9/12/19 4:59 PM, Matthias J. Sax wrote: >>> Thanks for updating the KIP. >>> >>> I agree with John that we (ie, you :)) can start a vote. >>> >>> >>> -Matthias >>> >>> On 9/11/19 9:23 AM, John Roesler wrote: >>>> Thanks for the update, Jukka! >>>> >>>> I'd be in favor of the current proposal. Not sure how the others feel. >>>> If people generally feel positive, it might be time to start a vote. >>>> >>>> Thanks, >>>> -John >>>> >>>> On Sat, Sep 7, 2019 at 12:40 AM Jukka Karvanen >>>> <jukka.karva...@jukinimi.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> Sorry; I need to rollback right away the one method removal what I was >>>>> proposing. >>>>> >>>>> One constructor with Long restored to TestRecord, which is needed by >>>>> TestInputTopic. >>>>> >>>>> Regards, >>>>> Jukka >>>>> >>>>> la 7. syysk. 2019 klo 8.06 Jukka Karvanen (jukka.karva...@jukinimi.com >> ) >>>>> kirjoitti: >>>>> >>>>>> 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> >>>>>>> >>>>>> >>>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
