You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "Rancho-7 (via GitHub)" <gi...@apache.org> on 2025/02/03 06:53:00 UTC
[PR] KAFKA-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Rancho-7 opened a new pull request, #18782:
URL: https://github.com/apache/kafka/pull/18782
jira: https://issues.apache.org/jira/browse/KAFKA-18694
As title.
### 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-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov merged PR #18782:
URL: https://github.com/apache/kafka/pull/18782
--
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-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Posted by "Rancho-7 (via GitHub)" <gi...@apache.org>.
Rancho-7 commented on PR #18782:
URL: https://github.com/apache/kafka/pull/18782#issuecomment-2630424988
> Thanks for the patch. I have some minor comments.
Thanks for the comments.Fixed.
--
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-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #18782:
URL: https://github.com/apache/kafka/pull/18782#issuecomment-2637036702
How are you evaluating if these classes can be safely converted? There are clear differences in behavior in equals/hashCode after these changes.
--
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-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Posted by "m1a2st (via GitHub)" <gi...@apache.org>.
m1a2st commented on code in PR #18782:
URL: https://github.com/apache/kafka/pull/18782#discussion_r1939333176
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorLoader.java:
##########
@@ -49,51 +49,7 @@ public short unknownType() {
* Object that is returned as part of the future from load(). Holds the partition load time and the
* end time.
*/
- class LoadSummary {
- private final long startTimeMs;
- private final long endTimeMs;
- private final long schedulerQueueTimeMs;
- private final long numRecords;
- private final long numBytes;
-
- public LoadSummary(long startTimeMs, long endTimeMs, long schedulerQueueTimeMs, long numRecords, long numBytes) {
- this.startTimeMs = startTimeMs;
- this.endTimeMs = endTimeMs;
- this.schedulerQueueTimeMs = schedulerQueueTimeMs;
- this.numRecords = numRecords;
- this.numBytes = numBytes;
- }
-
- public long startTimeMs() {
- return startTimeMs;
- }
-
- public long endTimeMs() {
- return endTimeMs;
- }
-
- public long schedulerQueueTimeMs() {
- return schedulerQueueTimeMs;
- }
-
- public long numRecords() {
- return numRecords;
- }
-
- public long numBytes() {
- return numBytes;
- }
-
- @Override
- public String toString() {
- return "LoadSummary(" +
- "startTimeMs=" + startTimeMs +
- ", endTimeMs=" + endTimeMs +
- ", schedulerQueueTimeMs=" + schedulerQueueTimeMs +
- ", numRecords=" + numRecords +
- ", numBytes=" + numBytes + ")";
- }
Review Comment:
Record the default toString method will show "Square brackets" instead "Parentheses", ie. `LoadSummary(...)` will change to `LoadSummary[...]`, If won't broken any test, I think it will be OK.
--
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-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18782:
URL: https://github.com/apache/kafka/pull/18782#discussion_r1938906951
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorLoader.java:
##########
@@ -49,50 +49,7 @@ public short unknownType() {
* Object that is returned as part of the future from load(). Holds the partition load time and the
* end time.
*/
- class LoadSummary {
- private final long startTimeMs;
- private final long endTimeMs;
- private final long schedulerQueueTimeMs;
- private final long numRecords;
- private final long numBytes;
-
- public LoadSummary(long startTimeMs, long endTimeMs, long schedulerQueueTimeMs, long numRecords, long numBytes) {
- this.startTimeMs = startTimeMs;
- this.endTimeMs = endTimeMs;
- this.schedulerQueueTimeMs = schedulerQueueTimeMs;
- this.numRecords = numRecords;
- this.numBytes = numBytes;
- }
-
- public long startTimeMs() {
- return startTimeMs;
- }
-
- public long endTimeMs() {
- return endTimeMs;
- }
-
- public long schedulerQueueTimeMs() {
- return schedulerQueueTimeMs;
- }
-
- public long numRecords() {
- return numRecords;
- }
-
- public long numBytes() {
- return numBytes;
- }
-
- @Override
- public String toString() {
- return "LoadSummary(" +
- "startTimeMs=" + startTimeMs +
- ", endTimeMs=" + endTimeMs +
- ", schedulerQueueTimeMs=" + schedulerQueueTimeMs +
- ", numRecords=" + numRecords +
- ", numBytes=" + numBytes + ")";
- }
+ record LoadSummary(long startTimeMs, long endTimeMs, long schedulerQueueTimeMs, long numRecords, long numBytes) {
}
Review Comment:
ditto
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorExecutorImpl.java:
##########
@@ -31,17 +31,7 @@
import java.util.concurrent.RejectedExecutionException;
public class CoordinatorExecutorImpl<S extends CoordinatorShard<U>, U> implements CoordinatorExecutor<U> {
- private static class TaskResult<R> {
- final R result;
- final Throwable exception;
-
- TaskResult(
- R result,
- Throwable exception
- ) {
- this.result = result;
- this.exception = exception;
- }
+ private record TaskResult<R>(R result, Throwable exception) {
}
Review Comment:
Could we merge them into one line?
```suggestion
private record TaskResult<R>(R result, Throwable exception) { }
```
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/HdrHistogram.java:
##########
@@ -133,15 +133,7 @@ public double measurePercentile(long now, double percentile) {
* A simple tuple of a timestamp and a value. Can be used updating a value and recording the
* timestamp of the update in a single atomic operation.
*/
- private static final class Timestamped<T> {
-
- private final long timestamp;
- private final T value;
-
- private Timestamped(long timestamp, T value) {
- this.timestamp = timestamp;
- this.value = value;
- }
+ private record Timestamped<T>(long timestamp, T value) {
}
Review Comment:
ditto
--
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-18694: Migrate suitable classes to recorods in coordinator-common module [kafka]
Posted by "Rancho-7 (via GitHub)" <gi...@apache.org>.
Rancho-7 commented on PR #18782:
URL: https://github.com/apache/kafka/pull/18782#issuecomment-2639113421
> How are you evaluating if these classes can be safely converted? There are clear differences in behavior in equals/hashCode after these changes.
Hi @ijuma
Thanks for catching this issue.You make a good point about the different behaviors in equals/hashCode.
While the [Java SE docs](https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.3) mention these differences:
> - A method public final boolean equals(Object) that returns true if and only if the argument is an instance of R, and the current instance is equal to the argument instance at every record component of R; otherwise false is returned.
> - A method public final int hashCode() that returns a hash code value derived from the hash code values at every record component of R.
in our implementation we're actually comparing the object fields directly rather than using equals() for object comparison.E.g:
https://github.com/apache/kafka/blob/852f14065b9a7084560b4864eba30ac343bbd6a6/coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorExecutorImpl.java#L96
https://github.com/apache/kafka/blob/852f14065b9a7084560b4864eba30ac343bbd6a6/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java#L2505
And we don't invoke hashCode() directly either.
Based on this approach, I believe these conversions should maintain compatibility without issues.
--
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