Hi All, Inspired by the discussion in this thread, there is a new 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 can be discarded if KIP-470 get accepted. Even this KIP might be rejected, migrating from this classes to KIP-470 is rather straighforward. There are addon package which can be used with any Kafka version >=1.1.0.before these are included to release. See info: https://github.com/jukkakarvanen/kafka-streams-test-topics Maven package: https://mvnrepository.com/artifact/com.github.jukkakarvanen/kafka-streams-test-topics Best Regards, Jukka to 9. toukok. 2019 klo 15.51 Patrik Kleindl (pklei...@gmail.com) kirjoitti: > 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 > > > > > > > > > > > > > > >
