Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-18644: improve generic type names for internal FK-join classes #18700

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

KAFKA-18644: improve generic type names for internal FK-join classes #18700

wants to merge 1 commit into from

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented Jan 24, 2025

Naming of generic types is not consistent, and hard to read. Standardizing to use KLeft/VLeft for the left/primary table, and KRight/VRight for right/foreign table, and using VOut as join result type.

@mjsax mjsax added the streams label Jan 24, 2025
@mjsax mjsax force-pushed the kafka-18644-dls-type-safely-fk-generics branch from 02ae4b0 to e7bb9fa Compare January 24, 2025 23:16
mjsax
mjsax commented Jan 24, 2025
View reviewed changes
...apache/kafka/streams/kstream/internals/foreignkeyjoin/SubscriptionJoinProcessorSupplier.java
//from older SubscriptionWrapper versions to newer versions.
throw new UnsupportedVersionException("SubscriptionWrapper is of an incompatible version.");
}
final SubscriptionWrapper<KLeft> value = subscriptionWrapper(valueAndTimestamp);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small refactoring proposed by IntelliJ -- guess it make sense to extract into a helper method -- easier to read overall, as semantics are a little clearer.

...c/main/java/org/apache/kafka/streams/kstream/internals/foreignkeyjoin/CombinedKeySchema.java
@@ -50,17 +50,17 @@ public CombinedKeySchema(final Supplier<String> foreignKeySerdeTopicSupplier,
foreignKeySerializer = foreignKeySerde == null ? null : foreignKeySerde.serializer();
}

@SuppressWarnings("unchecked")
@SuppressWarnings({"unchecked", "resource"})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppression some IntellIj warnings about "potential resource leaks" for not using try-with-resource for the "Closable" Serdes we get.

Similar elsewhere.

@mjsax
Copy link
Member Author

mjsax commented Jan 26, 2025

Java 17:

FAILED ❌ CooperativeConsumerCoordinatorTest > testOutdatedCoordinatorAssignment()
FAILED ❌ SharePartitionManagerTest > testReplicaManagerFetchShouldProceed()
FAILED ❌ SharePartitionManagerTest > testDelayedInitializationShouldCompleteFetchRequest()
FAILED ❌ RemoteIndexCacheTest > testIndexFileAlreadyExistOnDiskButNotInCache()
Found 3 flaky test failures:
FLAKY ⚠️  KafkaShareConsumerTest > testVerifyFetchAndAcknowledgeSync()
FLAKY ⚠️  AbstractCoordinatorTest > testWakeupAfterSyncGroupReceivedExternalCompletion()
FLAKY ⚠️  StickyAssignorTest > testLargeAssignmentAndGroupWithNonEqualSubscription(boolean).hasConsumerRack = false

Java 23:

Found 3 test failures:
FAILED ❌ SharePartitionManagerTest > testReplicaManagerFetchShouldProceed()
FAILED ❌ SharePartitionManagerTest > testDelayedInitializationShouldCompleteFetchRequest()
FAILED ❌ MetricsDuringTopicCreationDeletionTest > "testMetricsDuringTopicCreateDelete(String).quorum=kraft"
Found 1 flaky test failures:
FLAKY ⚠️  StickyAssignorTest > testLargeAssignmentAndGroupWithNonEqualSubscription(boolean).hasConsumerRack = false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mjsax