Hi Jukka
Sorry, that was mostly what I had in mind, I didn't have enough time to
look through the KIP.
My question was also if this handling of topics wouldn't make more sense
even outside the TTD, for the general API.
regards
Patrik
On Thu, 9 May 2019 at 14:43, Jukka Karvanen <jukka.karva...@jukinimi.com>
wrote:
> Hi Patrick,
>
> Sorry, I need to clarify.
> In this current version of KIP in wiki, topic object are created with
> constructor where driver, topicName and serdes are provided.
>
> TestInputTopic<String, String> inputTopic = new TestInputTopic<String,
> String>(testDriver, INPUT_TOPIC, new Serdes.StringSerde(), new
> Serdes.StringSerde());
>
> So if TopologyTestDriver modified, this could be
>
> TestInputTopic<String, String> inputTopic =
> testDriver.getInputTopic(INPUT_TOPIC, new Serdes.StringSerde(), new
> Serdes.StringSerde());
>
> or preferrable if serders can be found:
>
> TestInputTopic<String, String> inputTopic =
> testDriver.getInputTopic(INPUT_TOPIC);
>
> This initialization done normally in test setup and after it can be used
> with topic object:
>
> inputTopic.pipeInput("Hello");
>
>
> Or did you mean something else?
>
> Jukka
>
>
>
>
> to 9. toukok. 2019 klo 15.14 Patrik Kleindl (pklei...@gmail.com)
> kirjoitti:
>
> > Hi Jukka
> > Regarding your comment
> > > If there would be a way to find out needed serders for the topic, it
> > would make API even simpler.
> > I was wondering if it wouldn't make more sense to have a "topic object"
> > including the Serdes and use this instead of only passing in the name as
> a
> > string everywhere.
> > From a low-level perspective Kafka does and should not care what is
> inside
> > the topic, but from a user perspective this information usually belongs
> > together.
> > Sidenote: Having topics as objects would probably also make it easier to
> > track them from the outside.
> > regards
> > Patrik
> >
> > On Thu, 9 May 2019 at 10:39, Jukka Karvanen <jukka.karva...@jukinimi.com
> >
> > wrote:
> >
> > > Hi,
> > >
> > > I will write new KIP for the TestTopologyDriver Input and Output
> > usability
> > > changes.
> > > It is out of the scope of the current title: "Helper classes to make it
> > > simpler to write test logic with TopologyTestDriver"
> > > and we can get back to this KIP if that alternative is not favored.
> > >
> > > So my original approach was not to modify existing classes, but if we
> end
> > > up modifing TTD, I would also change the
> > > way to instantiate these topics. We could add
> getInputTopic("my-topic") /
> > > getOutputTopic("my-topic") to TTD, so it would work
> > > same way as with getStateStore and related methods.
> > >
> > > If there would be a way to find out needed serders for the topic, it
> > would
> > > make API even simpler.
> > >
> > > Generally still as a end user, I would prefer not only swapping the
> > > ConsumerRecord and ProducerRecord, but having
> > > interface accepting and returning Record, not needing to think about
> are
> > > those ConsumerRecord or ProducerRecords.
> > > and that way would could use same classes to pipe in and assert the
> > > result.Something similar than "private final static class Record"
> > > in TopologyTestDriverTest.
> > >
> > > Jukka
> > >
> > > ke 8. toukok. 2019 klo 17.01 John Roesler (j...@confluent.io)
> kirjoitti:
> > >
> > > > Hi Jukka, thanks for the reply!
> > > >
> > > > I think this is a good summary (the discussion was getting a little
> > > > unwieldy. I'll reply inline.
> > > >
> > > > Also, thanks for clarify about your library vs. this KIP. That makes
> > > > perfect sense to me.
> > > > >
> > > > > 1. Add JavaDoc for KIP
> > > > >
> > > > > Is there a good example of KIP where Javadoc is included, so I can
> > > > follow?
> > > > > I create this KIP based on this as an example::
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-247%3A+Add+public+test+utils+for+Kafka+Streams
> > > > >
> > > > >
> > > > > Now added some comments to KIP page to clarify timestamp handling,
> > but
> > > I
> > > > > did not want to add full JavaDoc of each methods.
> > > > > Is that enough?
> > > >
> > > > That's fine. I was just trying to make the review process more
> > > > efficient for other reviewers (which makes getting your KIP accepted
> > > > more efficient). I reviewed a few recent KIPs, and, indeed, I see
> that
> > > > javadocs are not actually as common as I thought.
> > > >
> > > > > 2. TTD usability changes and swapping ConsumerRecord and
> > ProducerRecord
> > > > in
> > > > > APIs
> > > > >
> > > > > To my point of view only:
> > > > > - changing readRecord to return ConsumerRecord would cause we
> cannot
> > > use
> > > > > OutputVerifier
> > > >
> > > > Yes, we'd likely have to provide new methods in OutputVerifier to
> work
> > > > with ConsumerRecord. If you buy into the plan of deprecating most of
> > > > the current-style interactions, this wouldn't be that confusing,
> since
> > > > all the ProducerRecord verifications would be deprecated, and only
> the
> > > > ConsumerRecord verifications would remain "live".
> > > >
> > > > > - changing pipeInput to take in ProducerRecord, but not providing
> > easy
> > > > way
> > > > > to contruct those like ConsumerRecordFactory
> > > >
> > > > I didn't follow this as well. The ConsumerRecordFactory is there
> > > > because it's a pain to construct ConsumerRecords. Conversely,
> > > > ProducerRecord has many convenience constructors, so we wouldn't need
> > > > a factory at all. This is a net win for users, since there's less
> > > > surface area for them to deal with. Under my proposal, we'd deprecate
> > > > the whole ConsumerRecordFactory.
> > > >
> > > > Note that there's an "idea parity check" here: ConsumerRecords are
> > > > hard to construct because developers aren't meant to ever construct
> > > > them. They are meant to construct ProducerRecords, which is why it's
> > > > made easy. TTD has inverted the relationships of these classes, which
> > > > is why the ConsumerRecordFactory is necessary, but if we correct it,
> > > > and return to a "normal" interaction with the Client library, then we
> > > > don't need special support classes.
> > > >
> > > > > - if initializing ConsumerRecord to/from ProducerRecord in these
> > > > classes
> > > > > field by field contructor, there are risk new fields are not added
> to
> > > > this
> > > > > classes if there are changes in ProducerRecord or ConsumerRecord
> > > >
> > > > This risk seems pretty low, to be honest. We will have tests that
> > > > exercise this testing framework, so if anyone changes ProducerRecord
> > > > or ConsumerRecord, our tests will break. Since both libraries are
> > > > build together, the problem would be fixed before the change is ever
> > > > merged to trunk.
> > > >
> > > > > I would propose a separate KIP for these and probably other
> > > > enhanchements:
> > > > > -superclass or common interface for ConsumerRecord and
> ProducerRecord
> > > > > -contructors to ConsumerRecord and ProducerRecord to initialize
> with
> > > this
> > > > > superclass
> > > > > -modify OutputVerifier to work with both ConsumerRecord and
> > > > ProducerRecord
> > > > > -create new RecordFactory to replace ConsumerRecordFactory
> > > >
> > > > I understand your motivation to control the scope of this change, but
> > > > I actually think that it's better for user-facing design changes to
> > > > occur in fewer, bigger, chunks, rather than many small changes.
> People
> > > > will get fatigued if multiple releases in a row change the
> > > > test-support library from under their feet. Better to do it in one
> > > > shot.
> > > >
> > > > Plus, this is a design discussion. We need to include the whole scope
> > > > of the system in the design, or we may realize in Phase 3 that there
> > > > was some design error in Phase 1, since we were only designing
> > > > locally. This doesn't mean that we only need one Jira ticket, there
> > > > can be many in support of this KIP, or that we only need one PR, it's
> > > > certainly better to send multiple small PRs to decrease risk and ease
> > > > reviews. But the design discussion doesn't need to be fragmented
> > > > similarly.
> > > >
> > > > > 3. return null vs NoSuchElementException when empty queue
> > > > >
> > > > > Should this be also included to the above TTD usability changes?
> > > > > If single row read methods is changed to throw expectiong, it would
> > > > require
> > > > > addition of hasRecords to able to verified the empty queue
> scenarios.
> > > > > I do not know how to implement it currently without modifying TTD
> to
> > > > > provide some kind way to get the queue size or peak items.
> > > >
> > > > Yes, it's absolutely within bounds to propose changes to TTD to
> > > > support the ergonomic API you're proposing.
> > > >
> > > > > 4. IllegalArgumentException("topic doesn't exist")
> > > > > Is this worth separate ticket?
> > > >
> > > > This is your call. That was just an idea in response to your
> > experience.
> > > >
> > > > > 5. org.apache.kafka.streams.test vs org.apache.kafka.streams
> > > > >
> > > > > I was thinking org.apache.kafka.streams.test where also
> > OutputVerifier
> > > > and
> > > > > ConsumerRecordFactory exist would be more logical place, but
> > > > > I do not know is there some technical reasons why TTD are in
> > > > > org.apache.kafka.streams, not in org.apache.kafka.streams.test
> where
> > > > other
> > > > > classes are.
> > > > >
> > > > > Did I skip something?
> > > >
> > > > Ah, no, you're right. I'm not sure why that is. I admit it's
> > > > confusing. I don't think the package matters *that* much, just keep
> it
> > > > wherever you think is appropriate.
> > > >
> > > >
> > > > That's all! Thanks for entertaining my thoughts.
> > > > -John
> > > >
> > >
> >
>
