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 2021/03/31 04:27:12 UTC
[GitHub] [kafka] chia7712 opened a new pull request #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
chia7712 opened a new pull request #10446:
URL: https://github.com/apache/kafka/pull/10446
I noticed this issue when reviewing #10436.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r624518845
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -803,7 +803,14 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
new AlterConfigOp(newConfigEntry("min.insync.replicas", "2"), AlterConfigOp.OpType.SET),
new AlterConfigOp(newConfigEntry("unclean.leader.election.enable", ""), AlterConfigOp.OpType.DELETE)
)
- assertEquals(expectedConfigOps, alterConfigOps.asScala.toSet)
+ assertEquals(expectedConfigOps.size, alterConfigOps.size())
+ expectedConfigOps.foreach { expectedOp =>
+ val actual = alterConfigOps.asScala.find(_.configEntry().name() == expectedOp.configEntry().name())
Review comment:
Nit: no need for `()` in many places.
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -149,24 +149,28 @@ public boolean equals(Object o) {
ConfigEntry that = (ConfigEntry) o;
- return this.name.equals(that.name) &&
- this.value != null ? this.value.equals(that.value) : that.value == null &&
+ return Objects.equals(this.name, that.name) &&
Review comment:
`name` cannot be `null` right?
--
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 a change in pull request #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r612095615
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -149,24 +149,28 @@ public boolean equals(Object o) {
ConfigEntry that = (ConfigEntry) o;
- return this.name.equals(that.name) &&
- this.value != null ? this.value.equals(that.value) : that.value == null &&
Review comment:
This was a bug as it evaluated following conditions:
```java
that.value == null &&
this.isSensitive == that.isSensitive &&
this.isReadOnly == that.isReadOnly &&
this.source == that.source &&
Objects.equals(this.synonyms, that.synonyms);
```
and the expected behavior is `that.value == null`.
There are some tests depending on the "incorrect" equals.
--
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 a change in pull request #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r623905395
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -803,7 +803,14 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
new AlterConfigOp(newConfigEntry("min.insync.replicas", "2"), AlterConfigOp.OpType.SET),
new AlterConfigOp(newConfigEntry("unclean.leader.election.enable", ""), AlterConfigOp.OpType.DELETE)
)
- assertEquals(expectedConfigOps, alterConfigOps.asScala.toSet)
+ assertEquals(expectedConfigOps.size, alterConfigOps.size())
+ expectedConfigOps.foreach { expectedOp =>
+ val actual = alterConfigOps.asScala.find(_.configEntry().name() == expectedOp.configEntry().name())
+ assertNotEquals(actual, None)
+ assertEquals(expectedOp.opType, actual.get.opType)
+ assertEquals(expectedOp.configEntry.name, actual.get.configEntry.name)
+ assertEquals(expectedOp.configEntry.value, actual.get.configEntry.value)
+ }
Review comment:
The arguments used to create `expectedConfigOps` consists of only `opType`, `configEntry.name` and `configEntry.value`. The test was passed since the `equals` function did not compare all fields. This change keep same behavior as before.
--
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 merged pull request #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10446:
URL: https://github.com/apache/kafka/pull/10446
--
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 a change in pull request #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r612961485
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -149,24 +149,28 @@ public boolean equals(Object o) {
ConfigEntry that = (ConfigEntry) o;
- return this.name.equals(that.name) &&
- this.value != null ? this.value.equals(that.value) : that.value == null &&
Review comment:
@ijuma @dajac I filed a Jira for this PR as it includes a bug fix. Personally, it should be backport to all active branches. WDYT?
--
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 #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r605610597
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -188,6 +188,7 @@ public int hashCode() {
result = prime * result + (isReadOnly ? 1 : 0);
result = prime * result + source.hashCode();
result = prime * result + synonyms.hashCode();
+ result = prime * result + type.hashCode();
Review comment:
`hashCode` needs to be consistent with `equals`. If we add something here, we need to add it to `equals` too. Also makes sense to add to `toString` although it's not required for correctness. With regards to `documentation`, it probably makes sense even though it's a bit inefficient.
--
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 a change in pull request #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r607486103
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -167,6 +169,8 @@ public int hashCode() {
result = prime * result + (isReadOnly ? 1 : 0);
result = prime * result + source.hashCode();
result = prime * result + synonyms.hashCode();
+ result = prime * result + type.hashCode();
+ result = prime * result + documentation.hashCode();
Review comment:
nice point! will copy that!
--
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 #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#issuecomment-823838830
```
Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.integration.KTableKTableForeignKeyInnerJoinMultiIntegrationTest.shouldInnerJoinMultiPartitionQueryable
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication()
```
unrelated error. merge trunk to trigger QA again.
--
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 #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#issuecomment-822411562
```
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.integration.KTableKTableForeignKeyInnerJoinMultiIntegrationTest.shouldInnerJoinMultiPartitionQueryable
```
unrelated error
--
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] dajac commented on a change in pull request #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r604843035
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -188,6 +188,7 @@ public int hashCode() {
result = prime * result + (isReadOnly ? 1 : 0);
result = prime * result + source.hashCode();
result = prime * result + synonyms.hashCode();
+ result = prime * result + type.hashCode();
Review comment:
`documentation` is not here nor in `toString` neither. Should we add it as well?
--
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] dajac commented on a change in pull request #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r606094276
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -167,6 +169,8 @@ public int hashCode() {
result = prime * result + (isReadOnly ? 1 : 0);
result = prime * result + source.hashCode();
result = prime * result + synonyms.hashCode();
+ result = prime * result + type.hashCode();
+ result = prime * result + documentation.hashCode();
Review comment:
documentation field could be null so we must handle it 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 #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r623899890
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -803,7 +803,14 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
new AlterConfigOp(newConfigEntry("min.insync.replicas", "2"), AlterConfigOp.OpType.SET),
new AlterConfigOp(newConfigEntry("unclean.leader.election.enable", ""), AlterConfigOp.OpType.DELETE)
)
- assertEquals(expectedConfigOps, alterConfigOps.asScala.toSet)
+ assertEquals(expectedConfigOps.size, alterConfigOps.size())
+ expectedConfigOps.foreach { expectedOp =>
+ val actual = alterConfigOps.asScala.find(_.configEntry().name() == expectedOp.configEntry().name())
+ assertNotEquals(actual, None)
+ assertEquals(expectedOp.opType, actual.get.opType)
+ assertEquals(expectedOp.configEntry.name, actual.get.configEntry.name)
+ assertEquals(expectedOp.configEntry.value, actual.get.configEntry.value)
+ }
Review comment:
Why do we need 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] chia7712 commented on a change in pull request #10446: MINOR: [ConfigEntry.class] add 'type' to 'toString' and 'hashCode'
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#discussion_r606022989
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java
##########
@@ -188,6 +188,7 @@ public int hashCode() {
result = prime * result + (isReadOnly ? 1 : 0);
result = prime * result + source.hashCode();
result = prime * result + synonyms.hashCode();
+ result = prime * result + type.hashCode();
Review comment:
@dajac @ijuma thanks for your suggestions. will copy that!
--
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 #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#issuecomment-827478300
```
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()
```
unrelated error. @ijuma @dajac please take a look if you have free cycles.
--
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 #10446: KAFKA-12661 ConfigEntry#equal does not compare other fields when value is NOT null
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10446:
URL: https://github.com/apache/kafka/pull/10446#issuecomment-830741353
```
Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
```
unrelated error. Merge this PR to trunk.
--
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