KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API by C0urante · Pull Request #7392 · apache/kafka

C0urante · 2019-09-25T23:56:50Z

KAFKA-8945 Jira ticket
KAFKA-8947 JIra ticket

The changes here are simple:
• KAFKA-8945: a few checks to ensure that constructor parameters in the AbstractState and ConnectorHealth classes are non-null and non-empty are fixed. The current logic erroneously ensures that they are either null or empty.
• KAFKA-8947: a bug in how the Connect framework instantiates TaskState objects for use by REST extensions (specifically, incorrect order in arguments provided to a constructor) is fixed. The current logic, while erroneous, should never have arisen in the past by REST extension developers, as it would have been covered by KAFKA-8945.

The RestExtensionIntegrationTest is expanded on to test these changes in the wild, verifying that they work and preventing any future regressions.

This fix should be backported through to 2.0, when Connect REST extensions were initially introduced.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…onstructor parameter validation

C0urante · 2019-09-26T06:05:58Z

Retest this, please?

C0urante · 2019-09-26T20:29:56Z

Retest this, please?

C0urante · 2019-09-30T21:09:36Z

Retest this, please.

mageshn

@C0urante, thanks a lot for the fix and also adding more test coverage. The fix itself LGTM. I just had a minor comment on the tests.

mageshn · 2019-10-07T21:02:50Z

+        workerProps.put(REST_EXTENSION_CLASSES_CONFIG, IntegrationTestRestExtension.class.getName());
+
+        // build a Connect cluster backed by Kafka and Zk
+        connect = new EmbeddedConnectCluster.Builder()
+            .name("connect-cluster")
+            .numWorkers(1)
+            .numBrokers(1)
+            .workerProps(workerProps)
+            .build();
+
+        // start the clusters
+        connect.start();
+
+        WorkerHandle worker = connect.workers().stream()
+            .findFirst()
+            .orElseThrow(() -> new AssertionError("At least one worker handle should be available"));
+
+        waitForCondition(
+            this::extensionIsRegistered,
+            REST_EXTENSION_REGISTRATION_TIMEOUT_MS,
+            "REST extension was never registered"
+        );


I see that testImmediateRequestForListOfConnectors does almost the same thing. Can we perhaps refactor this?

C0urante · 2019-10-08T18:36:46Z

Thanks for the review, @mageshn. I've combined the two separate integration tests into one, and also fixed a small bug. Does this look alright to you?

mageshn

LGTM. Thanks a lot @C0urante.

C0urante · 2019-10-08T20:30:32Z

Thanks @mageshn 🙂

@rhauch would you mind taking a look at this? Hoping that the fix here can be merged in time for the 2.4 release.

rhauch

LGTM. Thanks, @C0urante!

…#7392) Fix bug in Connect REST extension API caused by invalid constructor parameter validation, and update integration test to play nicely with Jenkins Fix instantiation of TaskState objects by Connect framework. Author: Chris Egerton <chrise@confluent.io> Reviewers: Magesh Nandakumar <mageshn@confluent.io>, Randall Hauch <rhauch@gmail.com>

Fix bug in Connect REST extension API caused by invalid constructor parameter validation, and update integration test to play nicely with Jenkins Fix instantiation of TaskState objects by Connect framework. Author: Chris Egerton <chrise@confluent.io> Reviewers: Magesh Nandakumar <mageshn@confluent.io>, Randall Hauch <rhauch@gmail.com>

* KAFKA-8649: send latest commonly supported version in assignment (apache#7425) Reviewers: Matthias J. Sax <matthias@confluent.io> * KAFKA-8262, KAFKA-8263: Fix flaky test `MetricsIntegrationTest` (apache#6922) (apache#7457) - Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state. Reviewers: Guozhang Wang <wangguoz@gmail.com> * MINOR: clarify wording around fault-tolerant state stores (apache#7510) Reviewers: Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io> * MINOR: Port changes from trunk for test stability to 2.3 branch (apache#7424) Reviewer: Matthias J. Sax <matthias@confluent.io> * KAFKA-9014: Fix AssertionError when SourceTask.poll returns an empty list (apache#7491) Author: Konstantine Karantasis <konstantine@confluent.io> Reviewer: Randall Hauch <rhauch@gmail.com> * KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API (apache#7392) Fix bug in Connect REST extension API caused by invalid constructor parameter validation, and update integration test to play nicely with Jenkins Fix instantiation of TaskState objects by Connect framework. Author: Chris Egerton <chrise@confluent.io> Reviewers: Magesh Nandakumar <mageshn@confluent.io>, Randall Hauch <rhauch@gmail.com> * KAFKA-8340, KAFKA-8819: Use PluginClassLoader while statically initializing plugins (apache#7315) Added plugin isolation unit tests for various scenarios, with a `TestPlugins` class that compiles and builds multiple test plugins without them being on the classpath and verifies that the Plugins and DelegatingClassLoader behave properly. These initially failed for several cases, but now pass since the issues have been fixed. KAFKA-8340 and KAFKA-8819 are closely related, and this fix corrects the problems reported in both issues. Author: Greg Harris <gregh@confluent.io> Reviewers: Chris Egerton <chrise@confluent.io>, Magesh Nandakumar <mageshn@confluent.io>, Konstantine Karantasis <konstantine@confluent.io>, Randall Hauch <rhauch@gmail.com> * MINOR: log reason for fatal error in locking state dir (apache#7534) Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io> * KAKFA-8950: Fix KafkaConsumer Fetcher breaking on concurrent disconnect (apache#7511) The KafkaConsumer Fetcher can sometimes get into an invalid state where it believes that there are ongoing fetch requests, but in fact there are none. This may be caused by the heartbeat thread concurrently handling a disconnection event just after the fetcher thread submits a request which would cause the Fetcher to enter an invalid state where it believes it has ongoing requests to the disconnected node but in fact it does not. This is due to a thread safety issue in the Fetcher where it was possible for the ordering of the modifications to the nodesWithPendingFetchRequests to be incorrect - the Fetcher was adding it after the listener had already been invoked, which would mean that pending node never gets removed again. This PR addresses that thread safety issue by ensuring that the pending node is added to the nodesWithPendingFetchRequests before the listener is added to the future, ensuring the finally block is called after the node is added. Reviewers: Tom Lee, Jason Gustafson <jason@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com> * Fix bug in AssignmentInfo#encode and add additional logging (apache#7545) Same as apache#7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com> * Bump version to 2.3.1 * Update versions to 2.3.2-SNAPSHOT * HOTFIX: fix bug in VP test where it greps for the wrong log message (apache#7643) Reviewers: Guozhang Wang <wangguoz@gmail.com>

C0urante added 2 commits September 25, 2019 16:45

KAFKA-8945: Fix bug in Connect REST extension API caused by invalid c…

af99be8

…onstructor parameter validation

KAFKA-8945: Tweak integration test to play nicely with Jenkins

99a33df

KAFKA-8947: Fix instantiation of TaskState objects by Connect framework

681d231

C0urante changed the title ~~KAFKA-8945: Fix bug in Connect REST extension API caused by invalid constructor parameter validation~~ KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API Sep 26, 2019

KAFKA-8945: Remove commented-out imports

b0c63a8

mageshn reviewed Oct 7, 2019

View reviewed changes

KAFKA-8945: Address review comments

890474f

mageshn approved these changes Oct 8, 2019

View reviewed changes

rhauch approved these changes Oct 15, 2019

View reviewed changes

rhauch merged commit bcdc633 into apache:trunk Oct 15, 2019

C0urante deleted the kafka-8945 branch October 15, 2019 22:27

kkonstantine mentioned this pull request Mar 22, 2020

Fixes in org.apache.kafka.connect.health #7482

Closed

kkonstantine added the connect label Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API#7392

KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API#7392
rhauch merged 5 commits intoapache:trunkfrom
C0urante:kafka-8945

C0urante commented Sep 25, 2019 •

edited

Loading

Uh oh!

C0urante commented Sep 26, 2019

Uh oh!

C0urante commented Sep 26, 2019

Uh oh!

C0urante commented Sep 30, 2019

Uh oh!

mageshn left a comment

Uh oh!

mageshn Oct 7, 2019

Uh oh!

C0urante commented Oct 8, 2019

Uh oh!

mageshn left a comment

Uh oh!

C0urante commented Oct 8, 2019

Uh oh!

rhauch left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

C0urante commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

C0urante commented Sep 26, 2019

Uh oh!

C0urante commented Sep 26, 2019

Uh oh!

C0urante commented Sep 30, 2019

Uh oh!

mageshn left a comment

Choose a reason for hiding this comment

Uh oh!

mageshn Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

C0urante commented Oct 8, 2019

Uh oh!

mageshn left a comment

Choose a reason for hiding this comment

Uh oh!

C0urante commented Oct 8, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

C0urante commented Sep 25, 2019 •

edited

Loading