MINOR: MockClient's disconnect() method has two bugs by eribeiro · Pull Request #183 · apache/kafka

eribeiro · 2015-09-01T03:32:46Z

(and a prospetive refactoring)

First, it compares Strings using == instead of equals()
Second, it tries to remove a String from a Set. As disconnect() method is only called by a single method on SenderTest then it's safe to refactor it to make it both explicit that its argument is a node id and perform a Integer.valueOf() before trying to remove from ready.
Third, not a bug, but the Iterator logic can be simplified, shrinking the scope of the Iterator without changing the logic.

asfbot · 2015-09-01T03:50:21Z

kafka-trunk-git-pr #278 SUCCESS
This pull request looks good

eribeiro · 2015-09-01T09:40:12Z

*tries to remove a String from a Set of Integer.

ijuma · 2015-09-01T10:07:22Z

The changes look good, but it why was SenderTest passing before this change even though it calls this method a number of times? Ideally, we would change SenderTest to fail and then fix this and make it pass.

eribeiro · 2015-09-01T18:04:28Z

Hi @ijuma, it is passing because the disconnect() method is only used on a single test method so far, and there's no yet any assertion that relies on the ready collection having the correct values after the disconnect() has been called. Also, the == works because the JVM does a good job of interning the Strings, but this is not the right way of performing comparison between objects in Java.

All in all, those are bugs lurking to happen, and I only stumbled upon by chance as MockClient. Do you think it's worth trying to devise an assertion that expose at least the collection flaw? I am fine with this approach, committing as-is today, or even dropping this PR. :)

ijuma · 2015-09-01T18:32:16Z

I think it would be nice to have the assertion you suggest, if it's easy to do yes.

Whatever happens, the fixes in the PR are good and should be included.

eribeiro · 2015-09-01T18:48:41Z

okay 👍 I am gonna include an assertion yet today. :)

eribeiro · 2015-09-01T23:14:59Z

Hi @ijuma, I have exposed a internal field of MockClient (same spirit of public Queue<ClientRequest> requests(), and added a two assertions to flesh out the bug. See what you think. Cheers!

ijuma · 2015-09-01T23:16:46Z

Why not use the existing isReady method instead of adding this new method?

asfbot · 2015-09-01T23:38:20Z

kafka-trunk-git-pr #300 SUCCESS
This pull request looks good

eribeiro · 2015-09-02T22:36:14Z

Hi @ijuma, good question. In fact, I didn't want to create a new Node just to test it, but looking again I should have been done that. Gonna change now. Btw, did you see that now is not used by isReady?

ijuma · 2015-09-02T22:54:18Z

Sounds good. Yes, the mocks often ignore parameters if not needed for the existing tests.

eribeiro · 2015-09-03T00:03:34Z

@ijuma please, tell me if it's good. Thx!

ijuma · 2015-09-03T00:14:05Z

Well, your change looked good, but now I am seeing a number of new commits in this branch? Seems like a merge/rebase gone wrong.

eribeiro · 2015-09-03T00:15:45Z

Yeah, has just fixed that. Sorry!

ijuma · 2015-09-03T00:19:33Z

LGTM

asfbot · 2015-09-03T00:48:44Z

kafka-trunk-git-pr #329 FAILURE
Looks like there's a problem with this pull request

eribeiro · 2015-09-03T01:39:47Z

Urgh, the failures in the Jenkins build above looks unrelated to my changes (see below):

kafka.api.ProducerSendTest > testClose FAILED
    java.lang.AssertionError: Partition [topic,0] metadata not propagated after 5000 ms
        at org.junit.Assert.fail(Assert.java:88)
        at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:655)
        at kafka.utils.TestUtils$.waitUntilMetadataIsPropagated(TestUtils.scala:695)
        at kafka.utils.TestUtils$$anonfun$createTopic$1.apply(TestUtils.scala:199)
        at kafka.utils.TestUtils$$anonfun$createTopic$1.apply(TestUtils.scala:198)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
        at scala.collection.immutable.Range.foreach(Range.scala:141)
        at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
        at scala.collection.AbstractTraversable.map(Traversable.scala:105)
        at kafka.utils.TestUtils$.createTopic(TestUtils.scala:198)
        at kafka.api.ProducerSendTest.testClose(ProducerSendTest.scala:197)

So, I am rebasing again. Hope this helps.

asfbot · 2015-09-03T02:49:00Z

kafka-trunk-git-pr #331 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-09-03T04:13:23Z

kafka-trunk-git-pr #335 SUCCESS
This pull request looks good

asfbot · 2015-09-07T03:38:16Z

kafka-trunk-git-pr #370 FAILURE
Looks like there's a problem with this pull request

…e refactoring) * First, it compares Strings using `==` instead of `equals()` * Second, it tries to remove a String from a Set<Integer>. As disconnect() method is only called by a single method on SenderTest then it's safe to refactor it to make it both explicity that its argument is a node id and do a Integer.valueOf() before trying to remove from `ready`. * Third, not a bug, but the Iterator logic can be simplified, shrinking the scope of the Iterator without changing the logic.

Add a message to Assertion

asfbot · 2015-09-29T21:13:54Z

kafka-trunk-git-pr #607 FAILURE
Looks like there's a problem with this pull request

ijuma · 2016-01-06T10:32:00Z

@eribeiro, can you please close this as it has been merged?

…ic (apache#183) Enable RBAC authorization in brokers hosting RBAC metadata topic, with the limitation that brokers themselves can be authorized without RBAC, using ACLs or as super.users. SocketServer will start inter-broker listeners first, wait for Authorizer to load RBAC metadata from the Kafka topic and then start other listeners. This ensures that Authorizer is fully initialized before external users that rely on RBAC are authorized. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>

…he listTopics() API (apache#183) TICKET = LIKAFKA-37320 LI_DESCRIPTION = Making the listTopics() API more light-weight so that a kafka broker would only return the topic names in the MetadataResponse. The partitions and replicas info would not be included in the MetadataResponse for the listTopics() API. EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release.

…he listTopics() API (apache#289) This is a squash of 2 commits: * [LI-HOTFIX] Exclude the partitions info in the MetadataResponse for the listTopics() API (apache#183) * [LI-HOTFIX] Making the ExcludePartitions field in MetadataRequest an optional field (apache#193) Compared to the original implementation, which uses the topic names directly during `excludePartitions = true`, this new implementation will still visit the metadata cache in order to obtain the topic Id info. ==1st commit message== [LI-HOTFIX] Exclude the partitions info in the MetadataResponse for the listTopics() API (apache#183) TICKET = LIKAFKA-37320 LI_DESCRIPTION = Making the listTopics() API more light-weight so that a kafka broker would only return the topic names in the MetadataResponse. The partitions and replicas info would not be included in the MetadataResponse for the listTopics() API. EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release. ==2nd commit message== [LI-HOTFIX] Making the ExcludePartitions field in MetadataRequest an optional field (apache#193) TICKET = LIKAFKA-37320 LI_DESCRIPTION = After the change made in b28666d, an old client version sending a MetadataRequest without the "ExcludePartitions" field to a new kafka server version expecting the field will result in the exception below. This PR turns the "ExcludePartitions" field into an optional one introduced in KIP-482. org.apache.kafka.common.errors.InvalidRequestException: Error getting request for apiKey: METADATA, apiVersion: 9, connectionId: 10.154.165.183:16637-10.154.87.109:44876-2848, listenerName: ListenerName(SSL), principal: User:likafka-cruise-control:None:prod-ltx1 at org.apache.kafka.common.requests.RequestContext.parseRequest(RequestContext.java:70) ~[kafka-clients-2.4.1.26.jar:?] at kafka.network.RequestChannel$Request.<init>(RequestChannel.scala:89) ~[kafka_2.12-2.4.1.26.jar:?] at kafka.network.Processor.$anonfun$processCompletedReceives$1(SocketServer.scala:928) ~[kafka_2.12-2.4.1.26.jar:?] at kafka.network.Processor.$anonfun$processCompletedReceives$1$adapted(SocketServer.scala:909) ~[kafka_2.12-2.4.1.26.jar:?] at scala.collection.Iterator.foreach(Iterator.scala:941) [scala-library-2.12.10.jar:?] at scala.collection.Iterator.foreach$(Iterator.scala:941) [scala-library-2.12.10.jar:?] at scala.collection.AbstractIterator.foreach(Iterator.scala:1429) [scala-library-2.12.10.jar:?] at scala.collection.IterableLike.foreach(IterableLike.scala:74) [scala-library-2.12.10.jar:?] at scala.collection.IterableLike.foreach$(IterableLike.scala:73) [scala-library-2.12.10.jar:?] at scala.collection.AbstractIterable.foreach(Iterable.scala:56) [scala-library-2.12.10.jar:?] at kafka.network.Processor.processCompletedReceives(SocketServer.scala:909) [kafka_2.12-2.4.1.26.jar:?] at kafka.network.Processor.run(SocketServer.scala:799) [kafka_2.12-2.4.1.26.jar:?] at java.lang.Thread.run(Thread.java:834) [?:?] Caused by: org.apache.kafka.common.protocol.types.SchemaException: Error reading field '_tagged_fields': java.nio.BufferUnderflowException at org.apache.kafka.common.protocol.types.Schema.read(Schema.java:118) ~[kafka-clients-2.4.1.26.jar:?] at org.apache.kafka.common.protocol.ApiKeys.parseRequest(ApiKeys.java:327) ~[kafka-clients-2.4.1.26.jar:?] at org.apache.kafka.common.requests.RequestContext.parseRequest(RequestContext.java:65) ~[kafka-clients-2.4.1.26.jar:?] ... 12 more EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release. Co-authored-by: Lucas Wang <luwang@linkedin.com>

…meout (apache#183)

Running tests locally might fail due to Postgres container not being ready.

Add migration test for LE difference and source node down cases

eribeiro added 3 commits September 29, 2015 17:59

Add a new assertion to flesh out bug on disconnect()

e66d21c

Remove new method in favour of an already existing one

8696c35

Add a message to Assertion

ijuma mentioned this pull request Dec 9, 2015

KAFKA-2974; == is used incorrectly in a few places in Java code #652

Closed

stumped2 closed this Feb 2, 2016

patrik-marton pushed a commit to patrik-marton/kafka that referenced this pull request Mar 11, 2025

CSMDS-980: KAFKA-18875: Fix KRaft controller registration retry on ti…

ec2dec2

…meout (apache#183)

davide-armand pushed a commit to aiven/kafka that referenced this pull request Dec 1, 2025

fix(inkless:test): wait for Postgres container to be ready (apache#183)

8ee5863

Running tests locally might fail due to Postgres container not being ready.

fvaleri pushed a commit to fvaleri/kafka that referenced this pull request May 28, 2026

Merge pull request apache#183 from showuon/cl3_old_support_5

8d27428

Add migration test for LE difference and source node down cases

Conversation

eribeiro commented Sep 1, 2015

Uh oh!

asfbot commented Sep 1, 2015

Uh oh!

eribeiro commented Sep 1, 2015

Uh oh!

ijuma commented Sep 1, 2015

Uh oh!

eribeiro commented Sep 1, 2015

Uh oh!

ijuma commented Sep 1, 2015

Uh oh!

eribeiro commented Sep 1, 2015

Uh oh!

eribeiro commented Sep 1, 2015

Uh oh!

ijuma commented Sep 1, 2015

Uh oh!

asfbot commented Sep 1, 2015

Uh oh!

eribeiro commented Sep 2, 2015

Uh oh!

ijuma commented Sep 2, 2015

Uh oh!

eribeiro commented Sep 3, 2015

Uh oh!

ijuma commented Sep 3, 2015

Uh oh!

eribeiro commented Sep 3, 2015

Uh oh!

ijuma commented Sep 3, 2015

Uh oh!

asfbot commented Sep 3, 2015

Uh oh!

eribeiro commented Sep 3, 2015

Uh oh!

asfbot commented Sep 3, 2015

Uh oh!

asfbot commented Sep 3, 2015

Uh oh!

asfbot commented Sep 7, 2015

Uh oh!

asfbot commented Sep 29, 2015

Uh oh!

ijuma commented Jan 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants