MINOR: cleanup KStream JavaDocs (4/N) - stream-table-inner-join #18721

mjsax · 2025-01-28T01:57:23Z

No description provided.

mjsax · 2025-01-28T02:02:20Z

...ams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKTableJoinProcessor.java

@@ -42,24 +42,24 @@
 import static org.apache.kafka.streams.processor.internals.metrics.TaskMetrics.droppedRecordsSensor;
 import static org.apache.kafka.streams.state.ValueAndTimestamp.getValueOrNull;

-class KStreamKTableJoinProcessor<K1, K2, V1, V2, VOut> extends ContextualProcessor<K1, V1, K1, VOut> {
+class KStreamKTableJoinProcessor<KStream, VStream, KTable, VTable, VOut> extends ContextualProcessor<KStream, VStream, KStream, VOut> {


Flipping type order -- no splash radius -- change limited to this line only, but diff is weird.

Effectively renaming K1 -> KStream, V1 -> VStream, K2 -> KTable, and V2 -> VTable.

@bbejeck Mentioned on some other PR, that KStream (and KTable) is a confusing generic type var, as we also have interfaces KStream and KTable.

Might be good to rename this differently, but I am not sure how. Ideas?

KLeft and KRight?

Maybe.

I was thinking about StreamKey/StreamValue/TableKey/TableValue to highlight that it's two different "objects". Left/Right would not express this explicitly.

lucasbru

Thanks for the PR @mjsax, LGTM. Left some minor comment that you can decide yourself if you want to address them.

lucasbru · 2025-01-29T15:38:00Z

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

-     * For this case, all data of the stream will be redistributed through the repartitioning topic by writing all
-     * records to it, and rereading all records from it, such that the join input {@code KStream} is partitioned
-     * correctly on its key.
+     * Join records of this stream with {@link KTable}'s records using non-windowed inner equi join.


nit: I think usually it's written equi-join not equi join.

lucasbru · 2025-01-29T15:41:59Z

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

+     * smaller than the defined grace-period allows; these "late records" will be dropped, and not join computation
+     * is triggered.
+     * Using a versioned state store for the {@link KTable} also implies that the defined "history retention" provides
+     * a cut-off point, and "late record" will be dropped, not updating the {@link KTable} state.


Suggested change

* a cut-off point, and "late record" will be dropped, not updating the {@link KTable} state.

* a cut-off point, and late records will be dropped, not updating the {@link KTable} state.

lucasbru · 2025-01-29T15:42:47Z

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

+     * Note, that using a join grace-period introduces the notion of "late records", i.e., records with a timestamp
+     * smaller than the defined grace-period allows; these "late records" will be dropped, and not join computation
+     * is triggered.
+     * Using a versioned state store for the {@link KTable} also implies that the defined "history retention" provides


It's a bit unclear what you mean by history retention here. It's quoted and not mentioned before, not explained.

lucasbru · 2025-01-29T15:44:42Z

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

+     * Using a versioned state store for the {@link KTable} also implies that the defined "history retention" provides
+     * a cut-off point, and "late record" will be dropped, not updating the {@link KTable} state.
+
+     * <p>If a join grace-period is specified, the {@code KStream} will be materialized in a local state store.


It seems to me that this paragraph would better be placed before the explanation of versioned state stores, since it's explaining state stores generally?

It's this way on purpose. Note, it's about KStream side materialization, and we only materialize the stream, if a grace-period is specific -- but we only allow this, if the KTable is versioned.

For regular/non-versioned KTables, using a grace-period and materializing/buffering the stream records does not make sense -- if there not multiple versions per-key, there is no point in delaying the KTable lookup.

Thoughts?

Aah, yes. Sorry, I misread this paragraph, still thinking about the KTable. This makes sense.

lucasbru · 2025-01-29T20:50:53Z

Updates look good to me

mjsax · 2025-01-30T00:08:33Z

Java 17:

Found 1 test failures:
FAILED ❌ KafkaAdminClientTest > testAdminClientApisAuthenticationFailure()
Found 3 flaky test failures:
FLAKY ⚠️  EagerConsumerCoordinatorTest > testOutdatedCoordinatorAssignment()
FLAKY ⚠️  DefaultStateUpdaterTest > shouldRestoreActiveStatefulTaskThenUpdateStandbyTaskAndAgainRestoreActiveStatefulTask()
FLAKY ⚠️  DeleteSegmentsByRetentionTimeTest > executeTieredStorageTest(String, String).quorum=kraft.groupProtocol=consumer

Java 23:

Found 1 test failures:
FAILED ❌ KafkaAdminClientTest > testAdminClientApisAuthenticationFailure()
Found 4 flaky test failures:
FLAKY ⚠️  TransactionsTest > "testBumpTransactionalEpochWithTV2Disabled(String, String, boolean).quorum=kraft, groupProtocol=classic, isTV2Enabled=false"
FLAKY ⚠️  TransactionsTest > "testBumpTransactionalEpochWithTV2Disabled(String, String, boolean).quorum=kraft, groupProtocol=consumer, isTV2Enabled=false"
FLAKY ⚠️  PlaintextAdminIntegrationTest > testConsumerGroups(String, String).quorum=kraft.groupProtocol=consumer
FLAKY ⚠️  PlaintextAdminIntegrationTest > testConsumerGroups(String, String).quorum=kraft.groupProtocol=classic

Fix for KakfaAdminClientTest is already on the way: #18735

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

….java

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

….java revert to avoid conflicts in follow up PRs

mjsax added the streams label Jan 28, 2025

mjsax commented Jan 28, 2025

View reviewed changes

lucasbru approved these changes Jan 29, 2025

View reviewed changes

mjsax added 4 commits January 29, 2025 12:21

MINOR: cleanup KStream JavaDocs (4/N)

3f6454e

generics

2e70f02

generics

f397662

review comments

965da38

mjsax force-pushed the minor-update-kstreams-javadocs-4 branch from bfc0311 to 965da38 Compare January 29, 2025 20:29

mjsax mentioned this pull request Jan 29, 2025

KAFKA-18644: improve generic type names for KStreamImpl and KTableImpl #18722

Open

mjsax commented Jan 30, 2025

View reviewed changes

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java Outdated Show resolved Hide resolved

mjsax and others added 3 commits January 29, 2025 18:46

Update streams/src/main/java/org/apache/kafka/streams/kstream/KStream…

2f23927

….java

rephrase return statement

b9fb135

fix

13c1a32

mjsax commented Jan 30, 2025

View reviewed changes

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java Outdated Show resolved Hide resolved

mjsax and others added 2 commits January 29, 2025 19:19

Update streams/src/main/java/org/apache/kafka/streams/kstream/KStream…

3cbb0b0

….java revert to avoid conflicts in follow up PRs

minor

6f63a48

mjsax added the docs label Jan 30, 2025

mjsax added 2 commits January 30, 2025 09:54

update generic type name base on review

77ec779

more renaming

7d39853

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: cleanup KStream JavaDocs (4/N) - stream-table-inner-join #18721

MINOR: cleanup KStream JavaDocs (4/N) - stream-table-inner-join #18721

mjsax commented Jan 28, 2025

mjsax Jan 28, 2025

mjsax Jan 29, 2025

lucasbru Jan 29, 2025

mjsax Jan 30, 2025

lucasbru left a comment

lucasbru Jan 29, 2025

lucasbru Jan 29, 2025

lucasbru Jan 29, 2025

lucasbru Jan 29, 2025

mjsax Jan 29, 2025

lucasbru Jan 29, 2025

lucasbru commented Jan 29, 2025

mjsax commented Jan 30, 2025

	* a cut-off point, and "late record" will be dropped, not updating the {@link KTable} state.
	* a cut-off point, and late records will be dropped, not updating the {@link KTable} state.

MINOR: cleanup KStream JavaDocs (4/N) - stream-table-inner-join #18721

Are you sure you want to change the base?

MINOR: cleanup KStream JavaDocs (4/N) - stream-table-inner-join #18721

Conversation

mjsax commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasbru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasbru commented Jan 29, 2025

mjsax commented Jan 30, 2025