Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-30 Thread via GitHub 


chia7712 merged PR #16062:
URL: https://github.com/apache/kafka/pull/16062


-- 
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

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-30 Thread via GitHub 


chia7712 commented on PR #16062:
URL: https://github.com/apache/kafka/pull/16062#issuecomment-2140856964

   The failed test is traced by 
https://issues.apache.org/jira/browse/KAFKA-16866


-- 
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

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-30 Thread via GitHub 


chia7712 commented on code in PR #16062:
URL: https://github.com/apache/kafka/pull/16062#discussion_r1620696237


##
clients/src/test/java/org/apache/kafka/common/ClusterTest.java:
##
@@ -89,4 +92,74 @@ public void testReturnUnmodifiableCollections() {
 new PartitionInfo(TOPIC_B, 2, NODES[1], NODES, NODES)));
 }
 
+@Test
+public void testNotEquals() {
+String clusterId1 = "clusterId1";
+String clusterId2 = "clusterId2";
+Node node0 = new Node(0, "host0", 100);
+Node node1 = new Node(1, "host1", 100);
+Set partitions1 = Collections.singleton(new 
PartitionInfo("topic1", 0, node0, new Node[]{node0, node1}, new Node[]{node0}));
+Set partitions2 = Collections.singleton(new 
PartitionInfo("topic2", 0, node0, new Node[]{node1, node0}, new Node[]{node1}));
+Set unauthorizedTopics1 = Collections.singleton("topic1");
+Set unauthorizedTopics2 = Collections.singleton("topic2");
+Set invalidTopics1 = Collections.singleton("topic1");
+Set invalidTopics2 = Collections.singleton("topic2");
+Set internalTopics1 = Collections.singleton("topic3");
+Set internalTopics2 = Collections.singleton("topic4");
+Node controller1 = node0;

Review Comment:
   ditto. Could you please generate new node object?



-- 
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

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-29 Thread via GitHub 


chia7712 commented on code in PR #16062:
URL: https://github.com/apache/kafka/pull/16062#discussion_r1619294656


##
clients/src/test/java/org/apache/kafka/common/ClusterTest.java:
##
@@ -89,4 +93,74 @@ public void testReturnUnmodifiableCollections() {
 new PartitionInfo(TOPIC_B, 2, NODES[1], NODES, NODES)));
 }
 
+@Test
+public void testNotEquals() {
+String clusterId1 = "clusterId1";
+String clusterId2 = "clusterId2";
+Node node0 = new Node(0, "host0", 100);
+Node node1 = new Node(1, "host1", 100);
+Set partitions1 = Collections.singleton(new 
PartitionInfo("topic1", 0, node0, new Node[]{node0, node1}, new Node[]{node0}));
+Set partitions2 = Collections.singleton(new 
PartitionInfo("topic2", 0, node0, new Node[]{node1, node0}, new Node[]{node1}));
+Set unauthorizedTopics1 = Collections.singleton("topic1");
+Set unauthorizedTopics2 = Collections.singleton("topic2");
+Set invalidTopics1 = Collections.singleton("topic1");
+Set invalidTopics2 = Collections.singleton("topic2");
+Set internalTopics1 = Collections.singleton("topic3");
+Set internalTopics2 = Collections.singleton("topic4");
+Node controller1 = node0;
+Node controller2 = node1;
+Map topicIds1 = Collections.singletonMap("topic1", 
Uuid.randomUuid());
+Map topicIds2 = Collections.singletonMap("topic2", 
Uuid.randomUuid());
+
+Cluster cluster1 = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics1, invalidTopics1, internalTopics1, controller1, 
topicIds1);
+Cluster differentTopicIds = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics1, invalidTopics1, internalTopics1, controller1, 
topicIds2);
+Cluster differentController = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics1, invalidTopics1, internalTopics1, controller2, 
topicIds1);
+Cluster differentInternalTopics = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics1, invalidTopics1, internalTopics2, controller1, 
topicIds1);
+Cluster differentInvalidTopics = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics1, invalidTopics2, internalTopics1, controller1, 
topicIds1);
+Cluster differentUnauthorizedTopics = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics2, invalidTopics1, internalTopics1, controller1, 
topicIds1);
+Cluster differentPartitions = new Cluster(clusterId1, 
Collections.singletonList(node0), partitions2,
+unauthorizedTopics1, invalidTopics1, internalTopics1, controller1, 
topicIds1);
+Cluster differentNodes = new Cluster(clusterId1, Arrays.asList(node0, 
node1), partitions1,
+unauthorizedTopics1, invalidTopics1, internalTopics1, controller1, 
topicIds1);
+Cluster differentClusterId = new Cluster(clusterId2, 
Collections.singletonList(node0), partitions1,
+unauthorizedTopics1, invalidTopics1, internalTopics1, controller1, 
topicIds1);
+
+assertFalse(cluster1.equals(differentTopicIds));
+assertFalse(cluster1.equals(differentController));
+assertFalse(cluster1.equals(differentInternalTopics));
+assertFalse(cluster1.equals(differentInvalidTopics));
+assertFalse(cluster1.equals(differentUnauthorizedTopics));
+assertFalse(cluster1.equals(differentPartitions));
+assertFalse(cluster1.equals(differentNodes));
+assertFalse(cluster1.equals(differentClusterId));
+}
+
+@Test
+public void testEquals() {
+String clusterId1 = "clusterId1";
+Node node1 = new Node(1, "host0", 100);
+Node node1duplicate = new Node(1, "host0", 100);
+Set partitions1 = Collections.singleton(new 
PartitionInfo("topic1", 0, node1, new Node[]{node1}, new Node[]{node1}));
+Set partitions1duplicate = Collections.singleton(new 
PartitionInfo("topic1", 0, node1duplicate, new Node[]{node1duplicate}, new 
Node[]{node1duplicate}));
+Set unauthorizedTopics1 = Collections.singleton("topic1");
+Set invalidTopics1 = Collections.singleton("topic1");
+Set internalTopics1 = Collections.singleton("topic3");
+Node controller1 = node1;
+Node controller1duplicate = node1duplicate;
+Uuid topicId1 = Uuid.randomUuid();
+Map topicIds1 = Collections.singletonMap("topic1", 
topicId1);
+Map topicIds1duplicate = 
Collections.singletonMap("topic1", topicId1);
+
+Cluster cluster1 = new Cluster(clusterId1, 
Collections.singletonList(node1), partitions1, unauthorizedTopics1,
+invalidTopics1, internalTopics1, controller1, topicIds1);
+Cluster cluster1duplicate = new

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-28 Thread via GitHub 


ahuang98 commented on code in PR #16062:
URL: https://github.com/apache/kafka/pull/16062#discussion_r1617827013


##
clients/src/main/java/org/apache/kafka/common/PartitionInfo.java:
##
@@ -88,6 +90,28 @@ public Node[] offlineReplicas() {
 return offlineReplicas;
 }
 
+@Override
+public int hashCode() {
+return Objects.hash(topic, partition, leader, replicas, 
inSyncReplicas, offlineReplicas);

Review Comment:
   I think I understand what you're saying, even though `Arrays.hashCode()` is 
called on the params, internally it computes the hash by calling 
`element.hashCode()` on each param (`object#hashCode`). If this were called on 
an int array for instance, this would result in different hashes being produced 
no matter the equality of their contents. However, since this is called on our 
Node array which has an overridden hashCode, we get the expected results still.
   
   I see how 
   ```
   Objects.hash(topic, partition, leader, Arrays.hashCode(replicas), 
Arrays.hashCode(inSyncReplicas), Arrays.hashCode(offlineReplicas));
   ```
   is technically more correct/explicit, I'll make the change.



-- 
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

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-27 Thread via GitHub 


chia7712 commented on code in PR #16062:
URL: https://github.com/apache/kafka/pull/16062#discussion_r1616238052


##
clients/src/main/java/org/apache/kafka/common/PartitionInfo.java:
##
@@ -88,6 +90,28 @@ public Node[] offlineReplicas() {
 return offlineReplicas;
 }
 
+@Override
+public int hashCode() {
+return Objects.hash(topic, partition, leader, replicas, 
inSyncReplicas, offlineReplicas);

Review Comment:
   My point was that `Objects.hash` will call `object#hashCode` for those array 
object, and it will return the object reference. It seems to me that is not we 
expect, and we can fix it by following example:
   ```java
   Objects.hash(topic, partition, leader, Arrays.hashCode(replicas), 
Arrays.hashCode(inSyncReplicas), Arrays.hashCode(offlineReplicas));
   ```



-- 
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

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-24 Thread via GitHub 


ahuang98 commented on code in PR #16062:
URL: https://github.com/apache/kafka/pull/16062#discussion_r1614010394


##
clients/src/main/java/org/apache/kafka/common/PartitionInfo.java:
##
@@ -88,6 +90,28 @@ public Node[] offlineReplicas() {
 return offlineReplicas;
 }
 
+@Override
+public int hashCode() {
+return Objects.hash(topic, partition, leader, replicas, 
inSyncReplicas, offlineReplicas);

Review Comment:
   Thanks for the comment! From the docs it looks like `Objects.hash()` 
delegates to `Arrays.hashCode()` (when we're passing more than one Object) so 
these should be equivalent. 
   ```
  /**
   * Generates a hash code for a sequence of input values. The hash
   * code is generated as if all the input values were placed into an
   * array, and that array were hashed by calling {@link
   * Arrays#hashCode(Object[])}.
   * 
   * ...
   * ...
   * Warning: When a single object reference is supplied, the returned
   * value does not equal the hash code of that object reference. This
   * value can be computed by calling {@link #hashCode(Object)}.
   *
   * @param values the values to be hashed
   * @return a hash value of the sequence of input values
   * @see Arrays#hashCode(Object[])
   * @see List#hashCode
   */
   public static int hash(Object... values) {
   return Arrays.hashCode(values);
   }
   ```



-- 
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

Re: [PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-24 Thread via GitHub 


chia7712 commented on code in PR #16062:
URL: https://github.com/apache/kafka/pull/16062#discussion_r1613444662


##
clients/src/main/java/org/apache/kafka/common/PartitionInfo.java:
##
@@ -88,6 +90,28 @@ public Node[] offlineReplicas() {
 return offlineReplicas;
 }
 
+@Override
+public int hashCode() {
+return Objects.hash(topic, partition, leader, replicas, 
inSyncReplicas, offlineReplicas);

Review Comment:
   Should we use `Arrays.hashCode(replicas)` to make sure the hash code is 
generated by "content" rather than reference?



-- 
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

[PR] KAFKA-16833: Fixing PartitionInfo and Cluster equals and hashCode [kafka]

2024-05-23 Thread via GitHub 


ahuang98 opened a new pull request, #16062:
URL: https://github.com/apache/kafka/pull/16062

   *More detailed description of your change,
   if necessary. The PR title and PR message become
   the squashed commit message, so use a separate
   comment to ping reviewers.*
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### 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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org