From 854dd314da93bba231cdbc1b7edb3b53d45fb4f8 Mon Sep 17 00:00:00 2001 From: "Colin P. Mccabe" Date: Thu, 23 Apr 2020 12:58:26 -0700 Subject: [PATCH 1/2] MINOR: equals() should compare all fields for generated classes --- .../utils/ImplicitLinkedHashCollection.java | 20 +++-- .../ImplicitLinkedHashMultiCollection.java | 16 ++-- .../ImplicitLinkedHashCollectionTest.java | 82 ++++++++++++------- .../kafka/message/MessageDataGenerator.java | 14 +++- 4 files changed, 87 insertions(+), 45 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollection.java b/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollection.java index d8d8ecaac3cc9..e8b9c66103b29 100644 --- a/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollection.java +++ b/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 extends AbstractCollection { + /** + * 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. diff --git a/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java b/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java index b5ae8f9793a46..b95b7de21673b 100644 --- a/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java +++ b/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 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); diff --git a/clients/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollectionTest.java b/clients/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollectionTest.java index 5f49cfe1b91a2..d23018d40dbf9 100644 --- a/clients/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollectionTest.java +++ b/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 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 iter, Iterator 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 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,7 +424,7 @@ public void testListIteratorRemove() { } // Remove after previous() - assertEquals(2, iter.previous().val); + assertEquals(2, iter.previous().key); iter.remove(); assertTrue(iter.hasNext()); assertTrue(iter.hasPrevious()); @@ -417,7 +432,7 @@ public void testListIteratorRemove() { 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()); @@ -425,8 +440,8 @@ public void testListIteratorRemove() { 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()); @@ -434,7 +449,7 @@ public void testListIteratorRemove() { 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 existing, } existing.remove(new TestElement(element)); } + + @Test + public void testSameKeysDifferentValues() { + ImplicitLinkedHashCollection 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)); + } } diff --git a/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java b/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java index ae26ffd2cc476..a4e3c8e78aaa1 100644 --- a/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java +++ b/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java @@ -103,8 +103,12 @@ private void generateClass(Optional 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); } } From bfb8eb69ab30e1a48877df59bc20e4668b471bec Mon Sep 17 00:00:00 2001 From: "Colin P. Mccabe" Date: Thu, 23 Apr 2020 13:58:21 -0700 Subject: [PATCH 2/2] Remove unneeded parenthesis --- .../java/org/apache/kafka/message/MessageDataGenerator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java b/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java index a4e3c8e78aaa1..62440bd0d8b98 100644 --- a/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java +++ b/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java @@ -2017,7 +2017,7 @@ private void generateClassEquals(String className, StructSpec struct, if (!struct.fields().isEmpty()) { buffer.printf("%s other = (%s) obj;%n", className, className); for (FieldSpec field : struct.fields()) { - if ((!elementKeysAreEqual) || field.mapKey()) { + if (!elementKeysAreEqual || field.mapKey()) { generateFieldEquals(field); } }