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 2022/03/17 12:09:43 UTC
[GitHub] [kafka] fxbing opened a new pull request #11912: KAFKA-13752: Uuid compare using equals in java
fxbing opened a new pull request #11912:
URL: https://github.com/apache/kafka/pull/11912
*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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1072178459
> @fxbing Could you update the description of the PR as well?
done. PTAL
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1074444703
I'm surprised that this wasn't caught by spotbugs. Maybe we need to start enabling more spotbugs checks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829904200
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
As @jolshan say, MetadataResponseTest is less necessary since the server should be building the response, but doesn't hurt.
Do I need to add MetadataResponseTest？
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #11912:
URL: https://github.com/apache/kafka/pull/11912
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829771763
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -20,6 +20,7 @@
import org.apache.kafka.common.errors.UnsupportedVersionException;
import org.apache.kafka.common.message.MetadataRequestData;
import org.apache.kafka.common.protocol.ApiKeys;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Review comment:
nit: We usually put static imports after the imports. Could we move it down? I think that we could put it right before the other static imports.
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -74,20 +75,30 @@ public void testMetadataRequestVersion() {
@Test
public void testTopicIdAndNullTopicNameRequests() {
// Construct invalid MetadataRequestTopics. We will build each one separately and ensure the error is thrown.
- List<MetadataRequestData.MetadataRequestTopic> topics = Arrays.asList(
+ List<MetadataRequestData.MetadataRequestTopic> invalidTopics = Arrays.asList(
new MetadataRequestData.MetadataRequestTopic().setName(null).setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName(null),
new MetadataRequestData.MetadataRequestTopic().setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName("topic").setTopicId(Uuid.randomUuid()));
+ List<MetadataRequestData.MetadataRequestTopic> validTopics = Arrays.asList(
Review comment:
nit: Would it make sense to split this test into two tests, one for the valid cases and one for the invalid cases? I think that it would be easier to read.
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
Could we add a unit test to cover this one?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1072551230
> Overall, I think that the impact of the bug is small because we usually rely on Uuid.ZERO_UUID by default so comparing the references also works.
Yeah, I think that usually we will just have the default in this field, but I think it's good to use equals since creating a new zero UUID should also be valid.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071812161
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r831450841
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
@fxbing Could you add a unit test for this one 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829903239
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -20,6 +20,7 @@
import org.apache.kafka.common.errors.UnsupportedVersionException;
import org.apache.kafka.common.message.MetadataRequestData;
import org.apache.kafka.common.protocol.ApiKeys;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Review comment:
done
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -74,20 +75,30 @@ public void testMetadataRequestVersion() {
@Test
public void testTopicIdAndNullTopicNameRequests() {
// Construct invalid MetadataRequestTopics. We will build each one separately and ensure the error is thrown.
- List<MetadataRequestData.MetadataRequestTopic> topics = Arrays.asList(
+ List<MetadataRequestData.MetadataRequestTopic> invalidTopics = Arrays.asList(
new MetadataRequestData.MetadataRequestTopic().setName(null).setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName(null),
new MetadataRequestData.MetadataRequestTopic().setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName("topic").setTopicId(Uuid.randomUuid()));
+ List<MetadataRequestData.MetadataRequestTopic> validTopics = Arrays.asList(
Review comment:
done
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1070947835
Thank you for this PR, can you also add a test case in `MetadataRequestTest.testTopicIdAndNullTopicNameRequests`?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071816320
Ah yeah, that could be useful? I think it's less necessary since the server should be building the response, but doesn't hurt.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r831720057
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
done. PTAL @dajac
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan removed a comment on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
jolshan removed a comment on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071812161
I was thinking adding the test from the ticket and verifying it _**doesn't**_ return an 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071812161
I was thinking adding the test from the ticket and verifying it _**doesn't**_ return an 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829771763
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -20,6 +20,7 @@
import org.apache.kafka.common.errors.UnsupportedVersionException;
import org.apache.kafka.common.message.MetadataRequestData;
import org.apache.kafka.common.protocol.ApiKeys;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Review comment:
nit: We usually put static imports after the imports. Could we move it down? I think that we could put it right before the other static imports.
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -74,20 +75,30 @@ public void testMetadataRequestVersion() {
@Test
public void testTopicIdAndNullTopicNameRequests() {
// Construct invalid MetadataRequestTopics. We will build each one separately and ensure the error is thrown.
- List<MetadataRequestData.MetadataRequestTopic> topics = Arrays.asList(
+ List<MetadataRequestData.MetadataRequestTopic> invalidTopics = Arrays.asList(
new MetadataRequestData.MetadataRequestTopic().setName(null).setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName(null),
new MetadataRequestData.MetadataRequestTopic().setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName("topic").setTopicId(Uuid.randomUuid()));
+ List<MetadataRequestData.MetadataRequestTopic> validTopics = Arrays.asList(
Review comment:
nit: Would it make sense to split this test into two tests, one for the valid cases and one for the invalid cases? I think that it would be easier to read.
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
Could we add a unit test to cover this one?
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
I would add one to avoid a similar regression in the future.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1070947835
Thank you for this PR, can you also add a test case in `MetadataRequestTest.testTopicIdAndNullTopicNameRequests`?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1072177203
> @fxbing Thanks for the PR. Good catch! Overall, I think that the impact of the bug is small because we usually rely on `Uuid.ZERO_UUID` by default so comparing the references also works. @jolshan Do you confirm my assessment?
Indeed it is. But for `Uuid.ZERO_UUID` of the same process, the index is the same.For different processes, there are still differences. Is this the expected situation?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1072129252
@fxbing Could you update the description of the PR 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071008981
>
done, PTAL. @dengziming
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829903239
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -20,6 +20,7 @@
import org.apache.kafka.common.errors.UnsupportedVersionException;
import org.apache.kafka.common.message.MetadataRequestData;
import org.apache.kafka.common.protocol.ApiKeys;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Review comment:
done
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/MetadataRequestTest.java
##########
@@ -74,20 +75,30 @@ public void testMetadataRequestVersion() {
@Test
public void testTopicIdAndNullTopicNameRequests() {
// Construct invalid MetadataRequestTopics. We will build each one separately and ensure the error is thrown.
- List<MetadataRequestData.MetadataRequestTopic> topics = Arrays.asList(
+ List<MetadataRequestData.MetadataRequestTopic> invalidTopics = Arrays.asList(
new MetadataRequestData.MetadataRequestTopic().setName(null).setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName(null),
new MetadataRequestData.MetadataRequestTopic().setTopicId(Uuid.randomUuid()),
new MetadataRequestData.MetadataRequestTopic().setName("topic").setTopicId(Uuid.randomUuid()));
+ List<MetadataRequestData.MetadataRequestTopic> validTopics = Arrays.asList(
Review comment:
done
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
As @jolshan say, MetadataResponseTest is less necessary since the server should be building the response, but doesn't hurt.
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
As @jolshan say, MetadataResponseTest is less necessary since the server should be building the response, but doesn't hurt.
Do I need to add MetadataResponseTest？
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829904200
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
As @jolshan say, MetadataResponseTest is less necessary since the server should be building the response, but doesn't hurt.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#discussion_r829973928
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
##########
@@ -151,7 +151,7 @@ public Cluster buildCluster() {
if (metadata.error == Errors.NONE) {
if (metadata.isInternal)
internalTopics.add(metadata.topic);
- if (metadata.topicId() != null && metadata.topicId() != Uuid.ZERO_UUID) {
+ if (metadata.topicId() != null && !Uuid.ZERO_UUID.equals(metadata.topicId())) {
Review comment:
I would add one to avoid a similar regression in the future.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1072129252
@fxbing Could you update the description of the PR 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1074883379
Merged to trunk, 3.2 and 3.1.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan removed a comment on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
jolshan removed a comment on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071812161
I was thinking adding the test from the ticket and verifying it _**doesn't**_ return an 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fxbing commented on pull request #11912: KAFKA-13752: Uuid compare using equals in java
Posted by GitBox <gi...@apache.org>.
fxbing commented on pull request #11912:
URL: https://github.com/apache/kafka/pull/11912#issuecomment-1071008981
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org