You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "frankvicky (via GitHub)" <gi...@apache.org> on 2025/02/03 07:16:36 UTC
[PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
frankvicky opened a new pull request, #18783:
URL: https://github.com/apache/kafka/pull/18783
JIRA: KAFKA-18698
The following are the classes that might be converted to records:
```
server-common:
TopicIdPartition
RequestAndCompletionHandler
FinalizedFeatures
server:
Assignment
```
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2636267542
Heya @chia7712, are we cherry-picking these changes for the 4.0 release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1939585504
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
In `org.apache.kafka.server` there is `package-info.java`, I think it is public api, thus we should not modify the `toString` method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1941188389
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
I think yes ๐
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2637459446
Hi @ijuma
Thanks for the reminder. It's indeed an issue that I should research first, especially for such underlying implementation and hot path related change. I will keep that in mind. ๐๐ผ
Back to the question, I have read [Java 17 SE spec](https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.10). Following is the statement:
> Equality of an instance a of a record class R with another instance b of the same record class at a record component c is determined as follows:
If the type of the record component c is a reference type, equality is determined as follows: if the value of the component field c of both a and b is the null reference then true is returned; if the value of the component field c of either a or b, but not both, is the null reference then false is returned; otherwise equality is determined by invoking the equals method on the value of the component field c of a, with an argument that is the value of the component field c of b.
If the type of the record component c is a primitive type T, equality is determined as if by invoking the static method compare of the wrapper class corresponding to T ([ยง5.1.7](https://docs.oracle.com/javase/specs/jls/se17/html/jls-5.html#jls-5.1.7)), with the first argument given by the value of the component field c of a, and the second argument given by the value of the component field c of b; if the method would return 0 then true is returned, otherwise false is returned.
According to the spec, the record's `equals` method behavior is basically the same as what we implemented before.
Although primitive types don't directly use the `==` operator, the result is effectively the same. This can be seen in the `Integer.compare` implementation.
https://github.com/apache/kafka/blob/c215b8b645edf6769f4a8976edcf0cc380ded21b/server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java#L49-L54
**Integer.compare:**
```java
public static int compare(int x, int y) {
return (x < y) ? -1 : ((x == y) ? 0 : 1);
}
```
> A method public final int hashCode() that returns a hash code value derived from the hash code values at every record component of R.
The hash code value of an instance a of a record class at a record component c is as follows:
If the type of the record component c is a reference type, then the hash code value is determined as if by invoking the hashCode method on the value of the component field c of a.
According to the spec, the behavior of the record's `hashCode` method is basically the same as what we implemented before:
https://github.com/apache/kafka/blob/c215b8b645edf6769f4a8976edcf0cc380ded21b/server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java#L56-L59
Therefore, the changes to `record` should be behavior-compatible. As for performance, since the underlying implementation remains essentially the same, there should not be significant performance differences. ๐๐ผ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1939765619
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
I understand, so the cause to keep it as is is that it is public and not because a test is using it, okay
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov merged PR #18783:
URL: https://github.com/apache/kafka/pull/18783
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2646033413
@TaiJuWu I know about the spec, but the actual implementation details matter. So, you would want to write a benchmark and confirm - not simply rely on reading the specification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2646051954
I see.
I will prepare one ๐๐ผ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1939564760
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
Maybe I am missing something obvious, but why can't you just change the output of the test to adhere to the new format and use the new format?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2636952281
> are we cherry-picking these changes for the 4.0 release?
This is unrelated to bug fix, so I think it is unnecessary to backport to 4.0 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1938893604
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
`org.apache.kafka.server.AssignmentTest#testAssignmentToString` has an assertion for the `toString` output. Hence, I don't remove this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940723859
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
> not really, the pakcage-info.java doesn't simply indicate "public" APIs. for example, this [package-info.java](https://github.com/apache/kafka/blob/ad031b99d312cdfe452965c841e22c1ab391956e/clients/src/main/java/org/apache/kafka/common/cache/package-info.java#L19) is used to announce this package is NOT public APIs.
Thanks for @chia7712 point out, it seems that we still have to verify it through the documents. [Kafka KIP](https://cwiki.apache.org/confluence/display/kafka/kafka+improvement+proposals)
```
All of the following are public interfaces that people build around:
Binary log format
The network protocol and api behavior
Any class for which the build generates Javadoc. This includes (but is not limited to; look for javadoc {} sections in build.gradle for the complete list):
org/apache/kafka/common/serialization
org/apache/kafka/common
org/apache/kafka/common/errors
org/apache/kafka/clients/producer
org/apache/kafka/clients/consumer (eventually, once stable)
Configuration, especially client configuration
Monitoring
Command line tools and arguments
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940701966
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
> In org.apache.kafka.server there is package-info.java
btw, in the server-common module the package `org.apache.kafka.server` has no `package-info.java`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940704606
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
Sorry, there are two `TopicIdPartition`, I trace the wrong directory :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2646059254
Yes, I think a benchmark for `TopicIdPartition` would be a good one since the original implementation is pretty simple and if the generated implementation has additional fixed overhead, that would be observed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2637047546
I asked this in another similar PR, but how are we verifying that it's safe to make these changes? Also, for critical classes like `TopicIdPartition`, we need to verify there is no perf regression when it comes to `equals`/`hashCode` as these classes are used in hot paths as HashMap keys.
To be clear, I'm not saying these changes are not good, but we need to do the due diligence since records have a specific behavior that may not be the same as the existing classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940699726
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
> In org.apache.kafka.server there is package-info.java, I think it is public api, thus we should not modify the toString method
not really, the `pakcage-info.java` doesn't simply indicate "public" APIs. for example, this [package-info.java](https://github.com/apache/kafka/blob/ad031b99d312cdfe452965c841e22c1ab391956e/clients/src/main/java/org/apache/kafka/common/cache/package-info.java#L19) is used to announce this package is NOT public APIs.
Although it's not a public API, I prefer to maintain the existing format due to its long-standing presence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940723859
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
> not really, the pakcage-info.java doesn't simply indicate "public" APIs. for example, this [package-info.java](https://github.com/apache/kafka/blob/ad031b99d312cdfe452965c841e22c1ab391956e/clients/src/main/java/org/apache/kafka/common/cache/package-info.java#L19) is used to announce this package is NOT public APIs.
Although it's not a public API, I prefer to maintain the existing format due to its long-standing presence.
Thanks for @chia7712 point out, it seems that we still have to verify it through the documents. (Kafka KIP)[https://cwiki.apache.org/confluence/display/kafka/kafka+improvement+proposals]
```
All of the following are public interfaces that people build around:
Binary log format
The network protocol and api behavior
Any class for which the build generates Javadoc. This includes (but is not limited to; look for javadoc {} sections in build.gradle for the complete list):
org/apache/kafka/common/serialization
org/apache/kafka/common
org/apache/kafka/common/errors
org/apache/kafka/clients/producer
org/apache/kafka/clients/consumer (eventually, once stable)
Configuration, especially client configuration
Monitoring
Command line tools and arguments
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1938894196
##########
server/src/main/java/org/apache/kafka/server/Assignment.java:
##########
@@ -96,23 +57,6 @@ boolean valid(int nodeId, MetadataImage image) {
return Replicas.contains(partition.replicas, nodeId);
}
- @Override
- public boolean equals(Object o) {
- if (o == null || (!(o instanceof Assignment other))) return false;
- return topicIdPartition.equals(other.topicIdPartition) &&
- directoryId.equals(other.directoryId) &&
- submissionTimeNs == other.submissionTimeNs &&
- successCallback.equals(other.successCallback);
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicIdPartition,
- directoryId,
- submissionTimeNs,
- successCallback);
- }
-
@Override
public String toString() {
StringBuilder bld = new StringBuilder();
Review Comment:
Same reason: https://github.com/apache/kafka/pull/18783/files#r1938893604
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18783:
URL: https://github.com/apache/kafka/pull/18783#issuecomment-2646048265
> you would want to write a benchmark and confirm
@frankvicky Since we have many JIRA tickets related to rewriting code with record classes, it would be great to have a JMH benchmark to compare both equals and hashCode performance. This benchmark would also give us more confidence when merging other related PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940704606
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
Sorry, there are two `TopicIdPartition` files, I trace the wrong directory :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1940723859
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
> not really, the pakcage-info.java doesn't simply indicate "public" APIs. for example, this [package-info.java](https://github.com/apache/kafka/blob/ad031b99d312cdfe452965c841e22c1ab391956e/clients/src/main/java/org/apache/kafka/common/cache/package-info.java#L19) is used to announce this package is NOT public APIs.
Thanks for @chia7712 point out, it seems that we still have to verify it through the documents. (Kafka KIP)[https://cwiki.apache.org/confluence/display/kafka/kafka+improvement+proposals]
```
All of the following are public interfaces that people build around:
Binary log format
The network protocol and api behavior
Any class for which the build generates Javadoc. This includes (but is not limited to; look for javadoc {} sections in build.gradle for the complete list):
org/apache/kafka/common/serialization
org/apache/kafka/common
org/apache/kafka/common/errors
org/apache/kafka/clients/producer
org/apache/kafka/clients/consumer (eventually, once stable)
Configuration, especially client configuration
Monitoring
Command line tools and arguments
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1941025794
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
Okay, so the summary is the following and please correct me if I am wrong - we are sticking to the toString format because it has been like so for a while and not because this is a public-facing API requiring a KIP to change it. If this is the case, I am happy for this to go through!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18698: Migrate suitable classes to records in server and server-common modules [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18783:
URL: https://github.com/apache/kafka/pull/18783#discussion_r1939585504
##########
server-common/src/main/java/org/apache/kafka/server/common/TopicIdPartition.java:
##########
@@ -18,45 +18,15 @@
import org.apache.kafka.common.Uuid;
-import java.util.Objects;
-
/**
* Represents a partition using its unique topic Id and partition number.
+ * @param topicId Universally unique Id representing this topic partition.
+ * @param partitionId The partition Id.
*/
-public final class TopicIdPartition {
- private final Uuid topicId;
- private final int partitionId;
-
- public TopicIdPartition(Uuid topicId, int partitionId) {
- this.topicId = topicId;
- this.partitionId = partitionId;
- }
-
- /**
- * @return Universally unique Id representing this topic partition.
- */
- public Uuid topicId() {
- return topicId;
- }
-
- /**
- * @return The partition Id.
- */
- public int partitionId() {
- return partitionId;
- }
-
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof TopicIdPartition other)) return false;
- return other.topicId.equals(topicId) && other.partitionId == partitionId;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(topicId, partitionId);
- }
-
+public record TopicIdPartition(
+ Uuid topicId,
+ int partitionId
+) {
@Override
public String toString() {
return topicId + ":" + partitionId;
Review Comment:
In `org.apache.kafka.server` there is `package-info.java`, I think it is public api, thus we should not modify the toString method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org