Thanks for the update.
Few more questions:
MJS6: Would it be simpler to (in the long run) drop single-partition
mode all together? We would only keep it now for backward compatibility
if `new TopologyTestDriver()` is used, but would always use
multi-partition mode when then new builder is used?
It seems to add complexity to keep single-partition mode for the builder
case?
If we say, the new builder always does multi-partition mode, and we
deprecate/remove `new TopologyTestDriver()` later, single-partition mode
would get removed, too.
Or is there something specific about single-partition mode that makes it
worth keeping for the new builder case? I would believe, with the only
exception that multi-partition mode does create more tasks, if all
topics are defined with a single partition, the record piping would be
the same as in single-partition mode? Or is there a difference?
MJS7:
Update equals() and hashCode() to include the partition field. For records
created via existing constructors, partition defaults to 0, preserving backward
compatibility for existing assertEquals calls.
Should it default to `-1` (or `Optional.empty()`) instead of `0`? Making
the default `0` seems to not work with "multi-partition mode" but break
it -- as it would skip the hashing/partitioning, as `0` would be set
explicitly?
And for `-1` as default and topics with a single partition, `-1` will
get hashed/mapped to partition-0 correctly.
-Matthias
On 6/4/26 5:47 AM, Sébastien Viale wrote:
Hi,
Thanks for the feedback and suggestions.
MJS1:
I fixed the typo `woraround` => workaround`
MJS2:
I fixed the inconsistency:
- removed duplicate entry for getKeyValueStore
- added missing *WithHeaders variants for state store APIs
MJS3:
The existing usages of TopologyTestDriver remain fully unchanged. In
particular, current code relying on the default behavior (i.e., no explicit
topic declaration) will continue to work as before.
Topic declaration is only required when a test explicitly opts into
multi-partition behavior. Otherwise, topics continue to be created
implicitly with the current semantics.
We will introduce a TopologyTestDriverBuilder class:
public class TopologyTestDriverBuilder {
public TopologyTestDriverBuilder(Topology topology);
public TopologyTestDriverBuilder withConfig(Properties config);
public TopologyTestDriverBuilder withInitialWallClockTime(Instant
initialWallClockTime);
public TopologyTestDriverBuilder declareTopic(String topicName,
int partitions);
public TopologyTestDriver build();
}
In this model, multi-partition mode is implicitly enabled when at least one
topic is declared with a partition count greater than 1.
As a result, the explicit withMultiPartitionMode() is no longer needed.
The existing TopologyTestDriver constructors remain unchanged for backward
compatibility.
If it is ok for you all, deprecation of those constructors is intentionally
deferred to a follow-up KIP to keep the scope of this change focused.
MJS4:
For this KIP, we wanted initially to make the partitioning behavior
deterministic and test-controlled.
Supporting partitioner.class would indeed be a natural extension, and
aligns well with production behavior. We propose deferring this to a
follow-up KIP to keep this one focused
However, we will implement the internal routing logic with an internal
TopologyTestDriverPartitioner that implements Partitioner. Which would
provide a natural extension point for future support of custom partitioners.
MJS5:
-
Your proposal seems like the best compromise:
- - TestRecord.equals() and TestRecord.hashCode() will be updated to
include the partition field.
- For existing code using the current constructors (without a partition
argument), the partition defaults to 0, preserving backward compatibility
for existing assertEquals() assertions.
- - A new utility method TestRecord.equalsIgnorePartition(TestRecord
other) will
be added for cases where the test author does not want to assert on
partition placement.
- This allows use with assertTrue(expected.equalsIgnorePartition(actual)) when
partition is irrelevant to the test.
-
- The KIP has been updated.
-
- Cheers
Le mer. 3 juin 2026 à 22:56, Matthias J. Sax <[email protected]> a écrit :
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
