-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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.
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
KAFKA-18644: improve generic type names for internal FK-join classes #18700
base: trunk
Are you sure you want to change the base?
Conversation
02ae4b0
to
e7bb9fa
Compare
//from older SubscriptionWrapper versions to newer versions. | ||
throw new UnsupportedVersionException("SubscriptionWrapper is of an incompatible version."); | ||
} | ||
final SubscriptionWrapper<KLeft> value = subscriptionWrapper(valueAndTimestamp); |
There was a problem hiding this comment.
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.
@@ -50,17 +50,17 @@ public CombinedKeySchema(final Supplier<String> foreignKeySerdeTopicSupplier, | |||
foreignKeySerializer = foreignKeySerde == null ? null : foreignKeySerde.serializer(); | |||
} | |||
|
|||
@SuppressWarnings("unchecked") | |||
@SuppressWarnings({"unchecked", "resource"}) |
There was a problem hiding this comment.
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.
Java 17:
Java 23:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one comment for your consideration.
public class CombinedKey<KF, KP> { | ||
private final KF foreignKey; | ||
private final KP primaryKey; | ||
public class CombinedKey<KRight, KLeft> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird that right is left and left is right. But not sure if I have a solution as we cannot swap them. How about standardizing on KForeign
and KPrimary
? That seemed rather clear to me, but I may be lacking context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is on-purpose, because that is the actually order of both arguments of CombinedKey
, ie, especially how we serialize it (but also constructor argument order; and the constructor argument order is also the way it is, to express the serialization order), what is important as we apply range-scans on RocksDB using the combined-key. I was also first thinking about maybe swapping it, but in the end, I did keep as is, because it might be better to align the order?
About the names. I did the change on-purpose, because the variable names, already have good names, it it IMHO make it's very clear?
private final KRight foreignKey;
private final KLeft primaryKey;
plus:
CombinedKey(final KRight foreignKey, final KLeft primaryKey)
e7bb9fa
to
9bbe1ca
Compare
Naming of generic types is not consistent, and hard to read. Standardizing to use
KLeft
/VLeft
for the left/primary table, andKRight
/VRight
for right/foreign table, and usingVOut
as join result type.