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