Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-5170. KafkaAdminClientIntegration test should wait until metada… #2976

Closed
wants to merge 2 commits into from

Conversation

cmccabe
Copy link
Contributor

@cmccabe cmccabe commented May 4, 2017

…ta is propagated to all brokers

@asfbot
Copy link

asfbot commented May 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3499/
Test FAILed (JDK 7 and Scala 2.10).

* @return The leader of the partition.
*/
def waitUntilBrokerMetadataIsPropagated(servers: Seq[KafkaServer],
brokerList: Seq[String],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need brokerList when you already have servers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more flexible if we can specify which brokers we want to wait for. Also, this way is a lot easier since we don't have to pull the host:port pairs out of the KafkaServer objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply compare the broker id? Seems much simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is a lot simpler. Will change.

@asfbot
Copy link

asfbot commented May 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3505/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented May 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3496/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3508/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3517/
Test PASSed (JDK 8 and Scala 2.11).

@cmccabe
Copy link
Contributor Author

cmccabe commented May 5, 2017

Got a flaky test. Looks like KAFKA-4801

kafka.api.ConsumerBounceTest > testConsumptionWithBrokerFailures FAILED
    org.apache.kafka.clients.consumer.CommitFailedException: Commit cannot be completed since the group has already rebalanced and assigned the partitions to another member. This means that the time between subsequent calls to poll() was longer than the configured max.poll.interval.ms, which typically implies that the poll loop is spending too much time message processing. You can address this either by increasing the session timeout or by reducing the maximum size of batches returned in poll() with max.poll.records.

@cmccabe
Copy link
Contributor Author

cmccabe commented May 5, 2017

retest this please

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3511/
Test FAILed (JDK 7 and Scala 2.10).

@cmccabe
Copy link
Contributor Author

cmccabe commented May 5, 2017

retest this please

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3520/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3517/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3526/
Test FAILed (JDK 8 and Scala 2.11).

@ijuma
Copy link
Contributor

ijuma commented May 5, 2017

retest this please

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3526/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3532/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented May 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3523/
Test FAILed (JDK 8 and Scala 2.12).

TestUtils.waitUntilTrue(() => false == servers.exists(server => {
expectedBrokerIds == server.apis.metadataCache.getAliveBrokers.map(broker => broker.id)
}), "Timed out waiting for all servers to learn about the broker list.", timeout, 50)
}
Copy link
Contributor

@ijuma ijuma May 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified this a little and made the comparison not care about order.

    val expectedBrokerIds = servers.map(_.config.brokerId).toSet
    TestUtils.waitUntilTrue(() => servers.forall(server =>
      expectedBrokerIds == server.apis.metadataCache.getAliveBrokers.map(_.id).toSet
    ), "Timed out waiting for broker metadata to propagate to all servers", timeout)

Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Made a small change before merging to trunk (as per my comment).

@asfgit asfgit closed this in 05ea454 May 5, 2017
@cmccabe cmccabe deleted the KAFKA-5170 branch May 20, 2019 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants