AndrewJSchofield commented on code in PR #18512:
URL: https://github.com/apache/kafka/pull/18512#discussion_r1919837490
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRecord.java:
##########
@@ -45,18 +67,27 @@ public class CoordinatorRecord {
* @param key A non-null key.
Review Comment:
nit: `type` parameter missing from javadoc.
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRecord.java:
##########
@@ -71,22 +102,17 @@ public ApiMessageAndVersion value() {
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
-
- CoordinatorRecord record = (CoordinatorRecord) o;
-
- if (!Objects.equals(key, record.key)) return false;
- return Objects.equals(value, record.value);
+ CoordinatorRecord that = (CoordinatorRecord) o;
+ return type == that.type && Objects.equals(key, that.key) &&
Objects.equals(value, that.value);
}
@Override
public int hashCode() {
- int result = key.hashCode();
- result = 31 * result + (value != null ? value.hashCode() : 0);
- return result;
+ return Objects.hash(type, key, value);
}
@Override
public String toString() {
- return "CoordinatorRecord(key=" + key + ", value=" + value + ")";
+ return "CoordinatorRecord(short=" + type + ", key=" + key + ", value="
+ value + ")";
Review Comment:
`short=` should be `type=`
##########
share-coordinator/src/test/java/org/apache/kafka/coordinator/share/ShareCoordinatorRecordHelpersTest.java:
##########
@@ -51,13 +51,12 @@ public void testNewShareSnapshotRecord() {
.build()
);
- CoordinatorRecord expectedRecord = new CoordinatorRecord(
- new ApiMessageAndVersion(
- new ShareSnapshotKey()
- .setGroupId(groupId)
- .setTopicId(topicId)
- .setPartition(partitionId),
- (short) 0),
+ CoordinatorRecord expectedRecord = CoordinatorRecord.record(
+ (short) 0,
Review Comment:
Use the constant rather than the integer value?
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordHelpersTest.java:
##########
@@ -108,12 +108,11 @@ public void
testNewConsumerGroupMemberSubscriptionRecord() {
.setSupportedProtocols(protocols))
.build();
- CoordinatorRecord expectedRecord = new CoordinatorRecord(
- new ApiMessageAndVersion(
- new ConsumerGroupMemberMetadataKey()
- .setGroupId("group-id")
- .setMemberId("member-id"),
- (short) 5),
+ CoordinatorRecord expectedRecord = CoordinatorRecord.record(
+ (short) 5,
Review Comment:
Given that we have constants defined for these values now, maybe the tests
should exclusively use those instead of the numbers.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
