Hi Bill,
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.
Jukka
On Fri, 21 Jun 2019, 20:57 Bill Bejeck, <bbej...@gmail.com> 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
> >
>
