You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/10 04:44:17 UTC
[GitHub] [kafka] cmccabe opened a new pull request #8640: MINOR: Fix compare raw tagged fields
cmccabe opened a new pull request #8640:
URL: https://github.com/apache/kafka/pull/8640
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r436401747
##########
File path: generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java
##########
@@ -2033,15 +2033,21 @@ private void generateClassEquals(String className, StructSpec struct,
elementKeysAreEqual ? "elementKeysAreEqual" : "equals");
buffer.incrementIndent();
buffer.printf("if (!(obj instanceof %s)) return false;%n", className);
+ buffer.printf("%s other = (%s) obj;%n", className, className);
if (!struct.fields().isEmpty()) {
- buffer.printf("%s other = (%s) obj;%n", className, className);
for (FieldSpec field : struct.fields()) {
if (!elementKeysAreEqual || field.mapKey()) {
generateFieldEquals(field);
}
}
}
- buffer.printf("return true;%n");
+ if (elementKeysAreEqual) {
Review comment:
This function in `MessageDataGenerator` is used to generate `equals`, but it is also used to generate the `elementKeysAreEqual` function. The latter function does not consider unknown tagged fields, since they can't be part of the key.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 edited a comment on pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
chia7712 edited a comment on pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#issuecomment-626298211
just curious. As ```toStruct``` doesn't include ```_unknownTaggedFields```, the object_A which is NOT equal to object_B has *same* struct to object_B. Is it fine?
```
val a = generated object with _unknownTaggedFields
val b = generated object without _unknownTaggedFields
assertNotEquals(a, b)
assertEquals(a.toStruct, b.toStruct)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r436401514
##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -962,4 +962,23 @@ private void verifyWriteSucceeds(short version, Message message) throws Exceptio
assertEquals("Expected the serialized size to be " + size +
", but it was " + buf.position(), size, buf.position());
}
+
+ @Test
+ public void testCompareWithUnknownTaggedFields() throws Exception {
+ CreateTopicsRequestData createTopics = new CreateTopicsRequestData();
+ createTopics.setTimeoutMs(123);
+ CreateTopicsRequestData createTopics2 = new CreateTopicsRequestData();
+ createTopics2.setTimeoutMs(123);
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
+ createTopics.unknownTaggedFields();
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
Review comment:
No, this is intentional. Invoking `unknownTaggedFields` will create the list if it doesn't exist. There is an optimization where if the list is empty, as it will be the vast majority of the time, we just store null. Example:
```
@Override
public List<RawTaggedField> unknownTaggedFields() {
if (_unknownTaggedFields == null) {
_unknownTaggedFields = new ArrayList<>(0);
}
return _unknownTaggedFields;
}
```
Therefore it is good to test that messages with null here are equivalent (via `equals`) to messages that have an empty list here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r436401965
##########
File path: generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java
##########
@@ -2033,15 +2033,21 @@ private void generateClassEquals(String className, StructSpec struct,
elementKeysAreEqual ? "elementKeysAreEqual" : "equals");
buffer.incrementIndent();
buffer.printf("if (!(obj instanceof %s)) return false;%n", className);
+ buffer.printf("%s other = (%s) obj;%n", className, className);
if (!struct.fields().isEmpty()) {
- buffer.printf("%s other = (%s) obj;%n", className, className);
for (FieldSpec field : struct.fields()) {
if (!elementKeysAreEqual || field.mapKey()) {
generateFieldEquals(field);
}
}
}
- buffer.printf("return true;%n");
+ if (elementKeysAreEqual) {
Review comment:
Thanks for the clarification, makes sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#issuecomment-626298211
just cusious. As ```toStruct``` doesn't include ```_unknownTaggedFields```, the object_A which is NOT equal to object_B has *same* struct to object_B. Is it fine?
```
val a = generated object with _unknownTaggedFields
val b = generated object without _unknownTaggedFields
assertNotEquals(a, b)
assertEquals(a.toStruct, b.toStruct)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r432865628
##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -962,4 +962,23 @@ private void verifyWriteSucceeds(short version, Message message) throws Exceptio
assertEquals("Expected the serialized size to be " + size +
", but it was " + buf.position(), size, buf.position());
}
+
+ @Test
+ public void testCompareWithUnknownTaggedFields() throws Exception {
+ CreateTopicsRequestData createTopics = new CreateTopicsRequestData();
+ createTopics.setTimeoutMs(123);
+ CreateTopicsRequestData createTopics2 = new CreateTopicsRequestData();
+ createTopics2.setTimeoutMs(123);
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
+ createTopics.unknownTaggedFields();
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
Review comment:
Did you mean to make a change to `unknownTaggedFields` before the two asserts above?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #8640:
URL: https://github.com/apache/kafka/pull/8640
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#issuecomment-626483136
It would be useful to include a bit more motivation for this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r436860958
##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -962,4 +962,23 @@ private void verifyWriteSucceeds(short version, Message message) throws Exceptio
assertEquals("Expected the serialized size to be " + size +
", but it was " + buf.position(), size, buf.position());
}
+
+ @Test
+ public void testCompareWithUnknownTaggedFields() throws Exception {
+ CreateTopicsRequestData createTopics = new CreateTopicsRequestData();
+ createTopics.setTimeoutMs(123);
+ CreateTopicsRequestData createTopics2 = new CreateTopicsRequestData();
+ createTopics2.setTimeoutMs(123);
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
+ createTopics.unknownTaggedFields();
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
Review comment:
Added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r436401914
##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -962,4 +962,23 @@ private void verifyWriteSucceeds(short version, Message message) throws Exceptio
assertEquals("Expected the serialized size to be " + size +
", but it was " + buf.position(), size, buf.position());
}
+
+ @Test
+ public void testCompareWithUnknownTaggedFields() throws Exception {
+ CreateTopicsRequestData createTopics = new CreateTopicsRequestData();
+ createTopics.setTimeoutMs(123);
+ CreateTopicsRequestData createTopics2 = new CreateTopicsRequestData();
+ createTopics2.setTimeoutMs(123);
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
+ createTopics.unknownTaggedFields();
+ assertEquals(createTopics, createTopics2);
+ assertEquals(createTopics2, createTopics);
Review comment:
OK, can you please add a comment? It's not obvious why the accessor is being called.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#issuecomment-636350073
Sorry, I noticed a couple of things after approving, so would like clarification on those before we merge. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#discussion_r432865696
##########
File path: generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java
##########
@@ -2033,15 +2033,21 @@ private void generateClassEquals(String className, StructSpec struct,
elementKeysAreEqual ? "elementKeysAreEqual" : "equals");
buffer.incrementIndent();
buffer.printf("if (!(obj instanceof %s)) return false;%n", className);
+ buffer.printf("%s other = (%s) obj;%n", className, className);
if (!struct.fields().isEmpty()) {
- buffer.printf("%s other = (%s) obj;%n", className, className);
for (FieldSpec field : struct.fields()) {
if (!elementKeysAreEqual || field.mapKey()) {
generateFieldEquals(field);
}
}
}
- buffer.printf("return true;%n");
+ if (elementKeysAreEqual) {
Review comment:
What is the purpose of this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8640: MINOR: equals() should check _unknownTaggedFields
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8640:
URL: https://github.com/apache/kafka/pull/8640#issuecomment-636349596
@cmccabe please include your clarification in the PR description and commit description.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org