KAFKA-9329. KafkaController::replicasAreValid should return error by soondenana · Pull Request #7865 · apache/kafka

soondenana · 2019-12-23T21:36:39Z

The KafkaController::replicasAreValid method currently returns a
boolean indicating if replicas are valid or not. But the failure
condition loses any context on why replicas are not valid. This change
updates the metod to return the error conition if validation fails. This
allows caller to report the error to the client.

The change also renames the replicasAreValid method to
validateReplicas to reflect updated semantics.

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)

soondenana · 2019-12-30T22:53:51Z

retest this please

soondenana · 2020-01-02T23:09:22Z

retest this please

soondenana · 2020-01-07T00:12:01Z

retest this please

soondenana · 2020-01-07T18:08:43Z

retest this please

soondenana · 2020-01-08T17:53:10Z

retest this please

hachikuji · 2020-01-11T04:37:01Z

retest this please

soondenana · 2020-01-13T17:13:46Z

2.13 build failed with OOM:

20:38:10 [0.036s][warning][os,thread] Failed to start thread - pthread_create failed (EAGAIN) for attributes: stacksize: 1024k, guardsize: 0k, detached.
20:38:10 Error occurred during initialization of VM
20:38:10 java.lang.OutOfMemoryError: unable to create native thread: possibly out of memory or process/resource limits reached

2.12 has two unit test failure:

kafka.api.GroupEndToEndAuthorizationTest.testNoConsumeWithoutDescribeAclViaSubscribe
kafka.api.PlaintextAdminIntegrationTest.testInvalidAlterPartitionReassignments

They pass locally. I will trigger tests again to see if 2.13 can make progress

soondenana · 2020-01-13T17:14:00Z

retest this please

hachikuji · 2020-01-14T01:54:52Z

Seems like a basic reassignment test case. Do you think it is not covered in ReassignPartitionsClusterTest? It's not too clear to me why we have two separate test cases.

It is covered. That code uses zkClient and ReassignPartitionsCommand. This test code uses AdminClient directly. Both of them test same backend code, so maybe its ok to get rid of this as when ReassignPartitionsCommand is fixed as part of KIP-500 we will have duplicate test.

Let me know what you prefer and I will take this test out.

hachikuji · 2020-01-15T01:44:47Z

retest this please

soondenana · 2020-01-15T18:34:30Z

2.13 build had issues when running kafka.admin.AclCommandTest and hung. It was killed:

22:15:53 Build timed out (after 270 minutes). Marking the build as aborted.

2.12 build had one test failure: PlaintextAdminIntegrationTest.testInvalidAlterPartitionReassignments. Pushed a fix for that.

hachikuji · 2020-01-16T23:33:05Z

retest this please

hachikuji · 2020-01-16T23:33:32Z

ok to test

hachikuji · 2020-01-16T23:51:38Z

I think we can remove this and just say there were duplicates in the replica list. Then we can remove the spotbugs warning.

hachikuji · 2020-01-16T23:53:02Z

Just a minor comment. Otherwise, looks good.

The `KafkaController::replicasAreValid` method currently returns a boolean indicating if replicas are valid or not. But the failure condition loses any context on why replicas are not valid. This change updates the metod to return the error conition if validation fails. This allows caller to report the error to the client. The change also renames the `replicasAreValid` method to `validateReplicas` to reflect updated semantics.

The change fixes the error when using map on `scala.collections.Seq` as 2.13 has following change: > In Scala 2.13 scala.Seq[+A] is an alias for > scala.collection.immutable.Seq[A], instead of scala.collection.Seq[A] Fixed code to specify correct collection type as implicit infrence was creating incorrect collection and failing compilation. The build still fails with coverage tool failing on deprecation warnings.

Couple of tests were duplicating code to create list of bootstrap servers. There is already a method in TestUtils to do the same, updated the tests to use that (TestUtils.bootstrapServers)

…assignments One failure case wasn't handled correctly in KafkaController::validateReplicas method. Updated code to check the boolean return condition and generate proper error message.

soondenana · 2020-01-17T17:48:57Z

ok to test

soondenana · 2020-01-17T17:49:53Z

retest this please

hachikuji · 2020-01-17T17:50:10Z

retest this please

hachikuji · 2020-01-17T17:50:44Z

ok to test

hachikuji · 2020-01-17T19:55:27Z

retest this please

hachikuji

LGTM

Conflicts or compilation errors due to the fact that we temporarily reverted the commit that removes Scala 2.11 support: * AclCommand.scala: take upstream changes. * AclCommandTest.scala: take upstream changes. * TransactionCoordinatorTest.scala: don't use SAMs, but adjust mock call to putTransactionStateIfNotExists given new signature. * TransactionStateManagerTest: use Runnable instead of SAMs. * PartitionLockTest: use Runnable instead of SAMs. * docs/upgrade.html: take upstream changes excluding line that states that Scala 2.11 support has been removed. * apache-github/trunk: (28 commits) KAFKA-9457; Fix flaky test org.apache.kafka.common.network.SelectorTest.testGracefulClose (apache#7989) MINOR: Update AclCommand help message to match implementation (apache#7990) MINOR: Update introduction page in Kafka documentation MINOR: Use Math.min for StreamsPartitionAssignor#updateMinReceivedVersion method (apache#7954) KAFKA-9338; Fetch session should cache request leader epoch (apache#7970) KAFKA-9329; KafkaController::replicasAreValid should return error message (apache#7865) KAFKA-9449; Adds support for closing the producer's BufferPool. (apache#7967) MINOR: Handle expandIsr in PartitionLockTest and ensure read threads not blocked on write (apache#7973) MINOR: Fix typo in connect integration test class name (apache#7976) KAFKA-9218: MirrorMaker 2 can fail to create topics (apache#7745) KAFKA-8847; Deprecate and remove usage of supporting classes in kafka.security.auth (apache#7966) MINOR: Suppress DescribeConfigs Denied log during CreateTopics (apache#7971) [MINOR]: Fix typo in Fetcher comment (apache#7934) MINOR: Remove unnecessary call to `super` in `MetricConfig` constructor (apache#7975) MINOR: fix flaky StreamsUpgradeTestIntegrationTest (apache#7974) KAFKA-9431: Expose API in KafkaStreams to fetch all local offset lags (apache#7961) KAFKA-9235; Ensure transaction coordinator is stopped after replica deletion (apache#7963) KAFKA-9410; Make groupId Optional in KafkaConsumer (apache#7943) MINOR: Removed accidental double negation in error message. (apache#7834) KAFKA-6144: IQ option to query standbys (apache#7962) ...

soondenana force-pushed the KAFKA-9329 branch from 13e8f77 to df596c6 Compare January 2, 2020 23:05

seanli-rallyhealth reviewed Jan 6, 2020

View reviewed changes

Comment thread core/src/test/scala/integration/kafka/admin/ReassignPartitionsIntegrationTest.scala Outdated

soondenana force-pushed the KAFKA-9329 branch from df596c6 to 594a0ea Compare January 7, 2020 00:10

hachikuji reviewed Jan 14, 2020

View reviewed changes

soondenana force-pushed the KAFKA-9329 branch from 594a0ea to aec877a Compare January 14, 2020 20:55

soondenana force-pushed the KAFKA-9329 branch from aec877a to dbf2f25 Compare January 15, 2020 18:34

hachikuji reviewed Jan 16, 2020

View reviewed changes

soondenana added 6 commits January 17, 2020 09:42

Update test to use TestUtils.bootstrapServers

7ef7fcb

Couple of tests were duplicating code to create list of bootstrap servers. There is already a method in TestUtils to do the same, updated the tests to use that (TestUtils.bootstrapServers)

Take care of review comments

f9cdc65

Fix failing PlaintextAdminIntegrationTest.testInvalidAlterPartitionRe…

6a95c2f

…assignments One failure case wasn't handled correctly in KafkaController::validateReplicas method. Updated code to check the boolean return condition and generate proper error message.

Remove use of collect to avoid spotbugs warning

bcd8a6f

soondenana force-pushed the KAFKA-9329 branch from dbf2f25 to bcd8a6f Compare January 17, 2020 17:47

hachikuji approved these changes Jan 17, 2020

View reviewed changes

hachikuji merged commit 9b312c4 into apache:trunk Jan 17, 2020

Conversation

soondenana commented Dec 23, 2019

Committer Checklist (excluded from commit message)

Uh oh!

soondenana commented Dec 30, 2019

Uh oh!

soondenana commented Jan 2, 2020

Uh oh!

Uh oh!

soondenana commented Jan 7, 2020

Uh oh!

soondenana commented Jan 7, 2020

Uh oh!

soondenana commented Jan 8, 2020

Uh oh!

hachikuji commented Jan 11, 2020

Uh oh!

soondenana commented Jan 13, 2020

Uh oh!

soondenana commented Jan 13, 2020

Uh oh!

Uh oh!

Uh oh!

hachikuji Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

soondenana Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Jan 15, 2020

Uh oh!

soondenana commented Jan 15, 2020

Uh oh!

hachikuji commented Jan 16, 2020

Uh oh!

hachikuji commented Jan 16, 2020

Uh oh!

hachikuji Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Jan 16, 2020

Uh oh!

soondenana commented Jan 17, 2020

Uh oh!

soondenana commented Jan 17, 2020

Uh oh!

hachikuji commented Jan 17, 2020

Uh oh!

hachikuji commented Jan 17, 2020

Uh oh!

hachikuji commented Jan 17, 2020

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants