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
