MINOR: Handle case where connector status endpoints returns 404 by wicknicks · Pull Request #6176 · apache/kafka

wicknicks · 2019-01-19T20:48:00Z

The integration test framework for Connect provides a org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster#connectorStatus method that checks the status of a Connector via the REST endpoint and deserializes the response into a ConnectorStateInfo object. If the connector creation request has just been finished, and task creation delayed for some reason (for example, rebalancing is ongoing), the REST API returns a 404 on the connectors/{connectorName}/status endpoint, and causes the test to fail with the exception:

[2019-01-19 03:47:59,289] (Test worker) ERROR Could not read connector state (org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster:178)
java.io.FileNotFoundException: http://localhost:38625/connectors/simple-conn/status
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1909)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1509)
	at org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster.executeGet(EmbeddedConnectCluster.java:223)
	at org.apache.kafka.connect.util.clusters.EmbeddedConnectCluster.connectorStatus(EmbeddedConnectCluster.java:176)
	at org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.lambda$testProduceConsumeConnector$1(ExampleConnectIntegrationTest.java:117)
	at org.apache.kafka.test.TestUtils.waitForCondition(TestUtils.java:355)
	at org.apache.kafka.test.TestUtils.waitForCondition(TestUtils.java:342)
	at org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testProduceConsumeConnector(ExampleConnectIntegrationTest.java:117)

This fix handles the 404 case instead of simply re-throwing the Exception.

Signed-off-by: Arjun Satish arjun@confluent.io

Committer Checklist (excluded from commit message)

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

wicknicks · 2019-01-19T21:15:01Z

@rhauch @mageshn @rayokota can you please take a look at this PR? Thanks in advance.

rhauch

A few questions.

rhauch · 2019-01-19T21:33:30Z

...ect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java

Do all the places where this method is called handle null? There is no JavaDoc that explains/summarizes what will be returned, so maybe add that JavaDoc. The AbstractHerder never returns null and instead throws a NotFoundException when the named connector is not found.

I removed this check, and documented the behavior. thanks!

rhauch · 2019-01-19T21:38:34Z

...ect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java

Nit: why use an else block here? The if block always returns, so really no else is needed, right?

The exception can contain any arbitrary status code. We are only calling the else block if the status code is not 404.

rhauch · 2019-01-19T21:43:21Z

...untime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java

This appears in the two connector integration tests in AK's codebase. Should TestUtils or EmbeddedConnectCluster have a method that does this?

Would it also be helpful here to catch any exceptions that might be thrown by the condition?

TestUtils is in the clients module. And adding it to EmbeddedConnectCluster seemed incorrect as this method depends on checking that depended on ConnectorHandle. I've moved it to an IntegrationTestUtils class. Let me know what you think.

rhauch

One more minor suggestion just to ward off a potentially alarming log message. Otherwise, looks good.

...runtime/src/test/java/org/apache/kafka/connect/integration/ErrorHandlingIntegrationTest.java

wicknicks · 2019-01-19T23:59:02Z

@rhauch I've addressed your comments. There is a bit of code duplication in the tests but we can address that in a follow up PR.

rhauch

Looks great, @wicknicks! Thanks for fixing this and addressing my nits and questions.

wicknicks · 2019-01-20T05:47:55Z

retest this please.

Signed-off-by: Arjun Satish <arjun@confluent.io>

wicknicks · 2019-01-20T09:56:43Z

retest this please.

…swallows exceptions Signed-off-by: Arjun Satish <arjun@confluent.io>

wicknicks · 2019-01-20T09:59:58Z

retest this please

Signed-off-by: Arjun Satish <arjun@confluent.io>

wicknicks · 2019-01-20T18:04:05Z

retest this please

mjsax · 2019-01-20T21:41:47Z

Ok. One last try. If it fails again, I will merge with one green build.

Retest this please.

mjsax · 2019-01-21T03:29:22Z

We got some green builds before. Merging as-is.

wicknicks · 2019-01-21T07:03:34Z

Thanks a lot, @mjsax!

Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

mjsax · 2019-01-21T08:08:05Z

Merged to trunk and cherry-picked to 2.1 and 2.0 branches.

…he#6176) Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

* ak/trunk: MINOR: fix race condition in KafkaStreamsTest (apache#6185) KAFKA-4850: Enable bloomfilters (apache#6012) MINOR: ducker-ak: add down -f, avoid using a terminal in ducker test KAFKA-5117: Stop resolving externalized configs in Connect REST API MINOR: Cleanup handling of mixed transactional/idempotent records (apache#6172) KAFKA-7844: Use regular subproject for generator to fix *All targets (apache#6182) Fix Documentation for cleanup.policy is out of date (apache#6181) MINOR: increase timeouts for KafkaStreamsTest (apache#6178) MINOR: Rejoin split ssl principal mapping rules (apache#6099) MINOR: Handle case where connector status endpoints returns 404 (apache#6176) MINOR: Remove unused imports, exceptions, and values (apache#6117) KAFKA-3522: Add internal RecordConverter interface (apache#6150) Fix Javadoc of KafkaConsumer (apache#6155) KAFKA-6455: Extend CacheFlushListener to forward timestamp (apache#6147) MINOR: Log partition info when creating new request batch in controller (apache#6145) KAFKA-7652: Part I; Fix SessionStore's findSession(single-key) (apache#6134) MINOR: Remove the InvalidTopicException handling in InternalTopicManager (apache#6167) [KAFKA-7024] Rocksdb state directory should be created before opening the DB (apache#6138) MINOR:: Fix typos (apache#6079)

…he#6176) Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

wicknicks force-pushed the connect-integration-connector-status branch 2 times, most recently from c1ed1c8 to 14f01d5 Compare January 19, 2019 20:58

wicknicks force-pushed the connect-integration-connector-status branch from 4358167 to 6c4b917 Compare January 19, 2019 21:36

rhauch reviewed Jan 19, 2019

View reviewed changes

wicknicks force-pushed the connect-integration-connector-status branch 2 times, most recently from 86b0161 to 4419f19 Compare January 19, 2019 23:30

rhauch reviewed Jan 19, 2019

View reviewed changes

...runtime/src/test/java/org/apache/kafka/connect/integration/ErrorHandlingIntegrationTest.java Outdated Show resolved Hide resolved

wicknicks force-pushed the connect-integration-connector-status branch from fcc6917 to a148226 Compare January 19, 2019 23:58

rhauch approved these changes Jan 20, 2019

View reviewed changes

wicknicks added 3 commits January 20, 2019 00:16

MINOR: Handle case where connector status endpoints returns 404

bc26cc9

Signed-off-by: Arjun Satish <arjun@confluent.io>

MINOR: Replace NotFoundException with ConnectRestException

2a0ea83

Signed-off-by: Arjun Satish <arjun@confluent.io>

MINOR: Address reviewer comments

1861d24

Signed-off-by: Arjun Satish <arjun@confluent.io>

wicknicks force-pushed the connect-integration-connector-status branch from a148226 to a6e21fc Compare January 20, 2019 08:25

MINOR: Add javadoc for checkForPartitionAssignment explaining why it …

f700c80

…swallows exceptions Signed-off-by: Arjun Satish <arjun@confluent.io>

wicknicks force-pushed the connect-integration-connector-status branch from a6e21fc to f700c80 Compare January 20, 2019 09:58

MINOR: Reword javadoc

fffef42

Signed-off-by: Arjun Satish <arjun@confluent.io>

mjsax merged commit dc935c4 into apache:trunk Jan 21, 2019

mjsax pushed a commit that referenced this pull request Jan 21, 2019

MINOR: Handle case where connector status endpoints returns 404 (#6176)

3c73633

Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

mjsax pushed a commit that referenced this pull request Jan 21, 2019

MINOR: Handle case where connector status endpoints returns 404 (#6176)

5d170e1

Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

abbccdda pushed a commit to abbccdda/kafka that referenced this pull request Jan 24, 2019

MINOR: Handle case where connector status endpoints returns 404 (apac…

ae026ca

…he#6176) Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019

MINOR: Handle case where connector status endpoints returns 404 (apac…

769c791

…he#6176) Reviewers: Randall Hauch <randall@confluent.io>, Matthias J. Sax <matthias@confluent.io>

kkonstantine added the connect label Oct 16, 2020

Conversation

wicknicks commented Jan 19, 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!

wicknicks commented Jan 19, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

rhauch Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

wicknicks Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

rhauch Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

wicknicks Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

rhauch Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

rhauch Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

wicknicks Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wicknicks commented Jan 19, 2019

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

wicknicks commented Jan 20, 2019

Uh oh!

wicknicks commented Jan 20, 2019

Uh oh!

wicknicks commented Jan 20, 2019

Uh oh!

wicknicks commented Jan 20, 2019

Uh oh!

mjsax commented Jan 20, 2019

Uh oh!

mjsax commented Jan 21, 2019

Uh oh!

wicknicks commented Jan 21, 2019

Uh oh!

mjsax commented Jan 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wicknicks commented Jan 19, 2019 •

edited

Loading