Thanks for the great discussion and KIP refinements.
Couple of comments:
MJS1: (nit) there is a typo `woraround`
MJS2: the KIP lists `getKeyValueStore` twice, and it's missing the newly
added `*WithHeader` variants.
MJS3: About the builder pattern. In general I like it, but it seems the
applied patter is a little bit odd, and also not backward compatible (or
maybe I misunderstand the proposal)?
Existing code, would only call `new TopologyTestDriver()`, and it seems
this code would break, and would need to get updated to add the
dedicated setup phase? W/o declaring the topics explicitly, it seems
later calls to `createInputTopic` and `createOutputTopic` would fail (at
least on what the KIP says I believe; could also be a phrasing problem
only)?
I also think, that we would need to have some "builder helper class".
Adding the methods to `TopologyTestDriver` can leave the object in an
weird state. A classic builder pattern would suggest, that we add
public class TTDBuilder { // or similar/better name
public TTDBuilder(Topology topology);
TTDBuilder withConfig(Properties config);
TTDBuilder withInitialWallClockTime(Instant initialWallClockTime);
TTDBuilder declareTopic(String topicName, int partitions);
TopologyTestDriver build();
}
For this case, I don't think that we even need `withMultiPartitionMode()`?
We would also deprecate the existing constructors `new
TopologyTestDriver`, which we still need to support thought. For `new
TopologyTestDriver()` we would just accept all topic names for
`createInputTopic` and `createOutputTopic` and create these topic with a
single partition. For this case, we would also still allow to use the
existing `getStateStore()` methods, which we might also want to deprecate?
(Maybe this is already proposed, but the KIP itself is not 100% clear
about it -- at least to me.)
MJS4: about partitioning; if the passed `Properties` configure
`partitioner.class`, I think we could just use it allowing to plugin a
global customer partitioner? Could also be a follow up KIP (maybe
including the ability to define a customer partitioner per input topic).
In general, I am wondering if we should implement the discussed
partition-routing strategy just as an internal `TDDPartitioner extends
Partitioner` (of course, this is an implementation detail). This would
setup the code nicely to support `partitioner.class` config.
MJS5: About testing and the new `partition` field on `TestRecord`. I
think that both `equals()` and `hashCode()` should be considered. For
existing code, all `TestRecord`s get set partition=0 and thus it's
backward compatible. For new code, checking the expected partition sound
desirable?
I still believe that for some tests, the partition is not necessary. For
this case, we could add `TestRecord.equalsIgnorePartition(TestRecord)`,
and users can use `assertTrue()` for this case?
-Matthias
On 6/1/26 10:46 AM, Sébastien Viale wrote:
Thanks for your response.
BB1:
I have removed the contradictory statement.
LB5:
OK, I will keep the KIP as it is.
I will call for the vote
Cheers
Le lun. 1 juin 2026 à 16:33, Bill Bejeck <[email protected]> a écrit :
Hi Sebastian,
Thanks for the KIP! This will be a great, much needed addition.
I realize I'm a bit late to the discussion. Overall things look good to me
and I have one nit comment. I also think the KIP is ready for a vote.
Regarding the treatment of null keys I agree with Lucas's comment for the
treatment of null keys (round-robin) distribution.
In "Partition Routing" section point 2 seems to reflection this change but
then then it concludes with "a null key goes to a default partition (0 in
the test driver)" . I find this a little contradictory, can we either
clarify or remove the statement?
Thanks,
Bill
On Mon, Jun 1, 2026 at 8:33 AM Lucas Brutschy via dev <
[email protected]>
wrote:
Hi Sebastian,
Thanks for the response!
LB5: I was thinking of this solution but I think it introduces more
problems than it solves. Long-term, I feel it would be a footgun if
people try to compare test records _with_ partitions. And the current
implementation with a reasonable default partition should already be
backward compatible. There are other possible solutions, like a custom
assertion or an operation to "wipe" the partition, but they are
probably not worth the effort. As I mentioned before, I don't see a
better solution to this -- I would just keep the KIP as it is. I
mostly included it in the discussion to make people aware and maybe
spark an idea in someone.
Cheers,
Lucas
