MINOR: equals() should compare all fields for generated classes

clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollection.java

@@ -46,11 +46,21 @@
  * This set does not allow null elements.  It does not have internal synchronization.
  */
 public class ImplicitLinkedHashCollection<E extends ImplicitLinkedHashCollection.Element> extends AbstractCollection<E> {
+    /**
+     * The interface which elements of this collection must implement.  The prev,
+     * setPrev, next, and setNext functions handle manipulating the implicit linked
+     * list which these elements reside in inside the collection.
+     * elementKeysAreEqual() is the function which this collection uses to compare
+     * elements.
+     */
     public interface Element {
         int prev();
         void setPrev(int prev);
         int next();
         void setNext(int next);
+        default boolean elementKeysAreEqual(Object other) {
+            return equals(other);
+        }
     }
 
     /**
@@ -313,7 +323,7 @@ final private int findIndexOfEqualElement(Object key) {
             if (element == null) {
                 return INVALID_INDEX;
             }
-            if (key.equals(element)) {
+            if (element.elementKeysAreEqual(key)) {
                 return slot;
             }
             slot = (slot + 1) % elements.length;
@@ -322,7 +332,7 @@ final private int findIndexOfEqualElement(Object key) {
     }
 
     /**
-     * An element e in the collection such that e.equals(key) and
+     * An element e in the collection such that e.elementKeysAreEqual(key) and
      * e.hashCode() == key.hashCode().
      *
      * @param key   The element to match.
@@ -348,7 +358,7 @@ final public int size() {
 
     /**
      * Returns true if there is at least one element e in the collection such
-     * that key.equals(e) and key.hashCode() == e.hashCode().
+     * that key.elementKeysAreEqual(e) and key.hashCode() == e.hashCode().
      *
      * @param key       The object to try to match.
      */
@@ -417,7 +427,7 @@ int addInternal(Element newElement, Element[] addElements) {
                 addElements[slot] = newElement;
                 return slot;
             }
-            if (element.equals(newElement)) {
+            if (element.elementKeysAreEqual(newElement)) {
                 return INVALID_INDEX;
             }
             slot = (slot + 1) % addElements.length;
@@ -441,7 +451,7 @@ private void changeCapacity(int newCapacity) {
     }
 
     /**
-     * Remove the first element e such that key.equals(e)
+     * Remove the first element e such that key.elementKeysAreEqual(e)
      * and key.hashCode == e.hashCode.
      *
      * @param key       The object to try to match.

clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java

@@ -26,19 +26,19 @@
  * A memory-efficient hash multiset which tracks the order of insertion of elements.
  * See org.apache.kafka.common.utils.ImplicitLinkedHashCollection for implementation details.
  *
- * This class is a multi-set because it allows multiple elements to be inserted that are
- * equal to each other.
+ * This class is a multi-set because it allows multiple elements to be inserted that
+ * have equivalent keys.
  *
  * We use reference equality when adding elements to the set.  A new element A can
  * be added if there is no existing element B such that A == B.  If an element B
- * exists such that A.equals(B), A will still be added.
+ * exists such that A.elementKeysAreEqual(B), A will still be added.
  *
  * When deleting an element A from the set, we will try to delete the element B such
  * that A == B.  If no such element can be found, we will try to delete an element B
- * such that A.equals(B).
+ * such that A.elementKeysAreEqual(B).
  *
  * contains() and find() are unchanged from the base class-- they will look for element
- * based on object equality, not reference equality.
+ * based on object equality via elementKeysAreEqual, not reference equality.
  *
  * This multiset does not allow null elements.  It does not have internal synchronization.
  */
@@ -103,7 +103,7 @@ int findElementToRemove(Object key) {
             }
             if (key == element) {
                 return slot;
-            } else if (key.equals(element)) {
+            } else if (element.elementKeysAreEqual(key)) {
                 bestSlot = slot;
             }
             slot = (slot + 1) % elements.length;
@@ -113,7 +113,7 @@ int findElementToRemove(Object key) {
 
     /**
      * Returns all of the elements e in the collection such that
-     * key.equals(e) and key.hashCode() == e.hashCode().
+     * key.elementKeysAreEqual(e) and key.hashCode() == e.hashCode().
      *
      * @param key       The element to match.
      *
@@ -130,7 +130,7 @@ final public List<E> findAll(E key) {
             if (element == null) {
                 break;
             }
-            if (key.equals(element)) {
+            if (key.elementKeysAreEqual(element)) {
                 @SuppressWarnings("unchecked")
                 E result = (E) elements[slot];
                 results.add(result);

clients/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollectionTest.java

@@ -47,9 +47,16 @@ public class ImplicitLinkedHashCollectionTest {
     final static class TestElement implements ImplicitLinkedHashCollection.Element {
         private int prev = ImplicitLinkedHashCollection.INVALID_INDEX;
         private int next = ImplicitLinkedHashCollection.INVALID_INDEX;
+        private final int key;
         private final int val;
 
-        TestElement(int val) {
+        TestElement(int key) {
+            this.key = key;
+            this.val = 0;
+        }
+
+        TestElement(int key, int val) {
+            this.key = key;
             this.val = val;
         }
 
@@ -73,22 +80,30 @@ public void setNext(int next) {
             this.next = next;
         }
 
+        @Override
+        public boolean elementKeysAreEqual(Object o) {
+            if (this == o) return true;
+            if ((o == null) || (o.getClass() != TestElement.class)) return false;
+            TestElement that = (TestElement) o;
+            return key == that.key;
+        }
+
         @Override
         public boolean equals(Object o) {
             if (this == o) return true;
             if ((o == null) || (o.getClass() != TestElement.class)) return false;
             TestElement that = (TestElement) o;
-            return val == that.val;
+            return key == that.key && val == that.val;
         }
 
         @Override
         public String toString() {
-            return "TestElement(" + val + ")";
+            return "TestElement(key=" + key + ", val=" + val + ")";
         }
 
         @Override
         public int hashCode() {
-            return val;
+            return key;
         }
     }
 
@@ -125,7 +140,7 @@ static void expectTraversal(Iterator<TestElement> iterator, Integer... sequence)
             Assert.assertTrue("Iterator yieled " + (i + 1) + " elements, but only " +
                     sequence.length + " were expected.", i < sequence.length);
             Assert.assertEquals("Iterator value number " + (i + 1) + " was incorrect.",
-                    sequence[i].intValue(), element.val);
+                    sequence[i].intValue(), element.key);
             i = i + 1;
         }
         Assert.assertTrue("Iterator yieled " + (i + 1) + " elements, but " +
@@ -140,7 +155,7 @@ static void expectTraversal(Iterator<TestElement> iter, Iterator<Integer> expect
                     i + " were expected.", expectedIter.hasNext());
             Integer expected = expectedIter.next();
             Assert.assertEquals("Iterator value number " + (i + 1) + " was incorrect.",
-                    expected.intValue(), element.val);
+                    expected.intValue(), element.key);
             i = i + 1;
         }
         Assert.assertFalse("Iterator yieled " + i + " elements, but at least " +
@@ -221,10 +236,10 @@ public void testSetViewModification() {
         assertEquals(3, set.size());
 
         // Ordering in the collection is maintained
-        int val = 3;
+        int key = 3;
         for (TestElement e : coll) {
-            assertEquals(val, e.val);
-            ++val;
+            assertEquals(key, e.key);
+            ++key;
         }
     }
 
@@ -236,9 +251,9 @@ public void testListViewGet() {
         coll.add(new TestElement(3));
 
         List<TestElement> list = coll.valuesList();
-        assertEquals(1, list.get(0).val);
-        assertEquals(2, list.get(1).val);
-        assertEquals(3, list.get(2).val);
+        assertEquals(1, list.get(0).key);
+        assertEquals(2, list.get(1).key);
+        assertEquals(3, list.get(2).key);
         assertEquals(3, list.size());
     }
 
@@ -259,13 +274,13 @@ public void testListViewModification() {
 
         // Removal from collection is reflected in list
         coll.remove(new TestElement(1));
-        assertEquals(3, list.get(0).val);
+        assertEquals(3, list.get(0).key);
         assertEquals(1, list.size());
 
         // Addition to collection is reflected in list
         coll.add(new TestElement(4));
-        assertEquals(3, list.get(0).val);
-        assertEquals(4, list.get(1).val);
+        assertEquals(3, list.get(0).key);
+        assertEquals(4, list.get(1).key);
         assertEquals(2, list.size());
     }
 
@@ -322,52 +337,52 @@ public void testListIteratorTraversal() {
         assertEquals(0, iter.nextIndex());
         assertEquals(-1, iter.previousIndex());
 
-        assertEquals(1, iter.next().val);
+        assertEquals(1, iter.next().key);
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(1, iter.nextIndex());
         assertEquals(0, iter.previousIndex());
 
-        assertEquals(2, iter.next().val);
+        assertEquals(2, iter.next().key);
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(2, iter.nextIndex());
         assertEquals(1, iter.previousIndex());
 
-        assertEquals(3, iter.next().val);
+        assertEquals(3, iter.next().key);
         assertFalse(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(3, iter.nextIndex());
         assertEquals(2, iter.previousIndex());
 
         // Step back to the middle of the list
-        assertEquals(3, iter.previous().val);
+        assertEquals(3, iter.previous().key);
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(2, iter.nextIndex());
         assertEquals(1, iter.previousIndex());
 
-        assertEquals(2, iter.previous().val);
+        assertEquals(2, iter.previous().key);
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(1, iter.nextIndex());
         assertEquals(0, iter.previousIndex());
 
         // Step forward one and then back one, return value should remain the same
-        assertEquals(2, iter.next().val);
+        assertEquals(2, iter.next().key);
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(2, iter.nextIndex());
         assertEquals(1, iter.previousIndex());
 
-        assertEquals(2, iter.previous().val);
+        assertEquals(2, iter.previous().key);
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(1, iter.nextIndex());
         assertEquals(0, iter.previousIndex());
 
         // Step back to the front of the list
-        assertEquals(1, iter.previous().val);
+        assertEquals(1, iter.previous().key);
         assertTrue(iter.hasNext());
         assertFalse(iter.hasPrevious());
         assertEquals(0, iter.nextIndex());
@@ -409,32 +424,32 @@ public void testListIteratorRemove() {
         }
 
         // Remove after previous()
-        assertEquals(2, iter.previous().val);
+        assertEquals(2, iter.previous().key);
         iter.remove();
         assertTrue(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(1, iter.nextIndex());
         assertEquals(0, iter.previousIndex());
 
         // Remove the first element of the list
-        assertEquals(1, iter.previous().val);
+        assertEquals(1, iter.previous().key);
         iter.remove();
         assertTrue(iter.hasNext());
         assertFalse(iter.hasPrevious());
         assertEquals(0, iter.nextIndex());
         assertEquals(-1, iter.previousIndex());
 
         // Remove the last element of the list
-        assertEquals(4, iter.next().val);
-        assertEquals(5, iter.next().val);
+        assertEquals(4, iter.next().key);
+        assertEquals(5, iter.next().key);
         iter.remove();
         assertFalse(iter.hasNext());
         assertTrue(iter.hasPrevious());
         assertEquals(1, iter.nextIndex());
         assertEquals(0, iter.previousIndex());
 
         // Remove the final remaining element of the list
-        assertEquals(4, iter.previous().val);
+        assertEquals(4, iter.previous().key);
         iter.remove();
         assertFalse(iter.hasNext());
         assertFalse(iter.hasPrevious());
@@ -556,4 +571,15 @@ private void removeRandomElement(Random random, Collection<Integer> existing,
         }
         existing.remove(new TestElement(element));
     }
+
+    @Test
+    public void testSameKeysDifferentValues() {
+        ImplicitLinkedHashCollection<TestElement> coll = new ImplicitLinkedHashCollection<>();
+        assertTrue(coll.add(new TestElement(1, 1)));
+        assertFalse(coll.add(new TestElement(1, 2)));
+        TestElement element2 = new TestElement(1, 2);
+        TestElement element1 = coll.find(element2);
+        assertFalse(element2.equals(element1));
+        assertTrue(element2.elementKeysAreEqual(element1));
+    }
 }

generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java

@@ -103,8 +103,12 @@ private void generateClass(Optional<MessageSpec> topLevelMessageSpec,
         generateClassToJson(className, struct, parentVersions);
         buffer.printf("%n");
         generateClassSize(className, struct, parentVersions);
+        if (isSetElement) {
+            buffer.printf("%n");
+            generateClassEquals(className, struct, true);
+        }
         buffer.printf("%n");
-        generateClassEquals(className, struct, isSetElement);
+        generateClassEquals(className, struct, false);
         buffer.printf("%n");
         generateClassHashCode(struct, isSetElement);
         buffer.printf("%n");
@@ -2003,15 +2007,17 @@ private void generateStringToBytes(String name) {
         buffer.printf("_cache.cacheSerializedValue(%s, _stringBytes);%n", name);
     }
 
-    private void generateClassEquals(String className, StructSpec struct, boolean onlyMapKeys) {
+    private void generateClassEquals(String className, StructSpec struct,
+                                     boolean elementKeysAreEqual) {
         buffer.printf("@Override%n");
-        buffer.printf("public boolean equals(Object obj) {%n");
+        buffer.printf("public boolean %s(Object obj) {%n",
+            elementKeysAreEqual ? "elementKeysAreEqual" : "equals");
         buffer.incrementIndent();
         buffer.printf("if (!(obj instanceof %s)) return false;%n", className);
         if (!struct.fields().isEmpty()) {
             buffer.printf("%s other = (%s) obj;%n", className, className);
             for (FieldSpec field : struct.fields()) {
-                if ((!onlyMapKeys) || field.mapKey()) {
+                if (!elementKeysAreEqual || field.mapKey()) {
                     generateFieldEquals(field);
                 }
             }