KAFKA-14785: Connect offset read REST API #13434

yashmayya · 2023-03-22T08:36:06Z

https://issues.apache.org/jira/browse/KAFKA-14785
KIP-875: First-class offsets support in Kafka Connect
Implements the new GET /connectors/{connector}/offsets REST API

Committer Checklist (excluded from commit message)

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

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/OffsetBackingStore.java

C0urante

Thanks Yash, this is looking great. I've finished a full pass of the changes to the main codebase; I'll probably wait to look at tests until the bigger questions/suggestions have been addressed.

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/FileOffsetBackingStore.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/FileOffsetBackingStore.java

…orOffsets; use connector class loader while retrieving offsets in Worker; refactor Worker::connectorOffsets to be an async API; use timeout for admin client request; move sink related keys from ConnectorOffset to SinkUtils; log and ignore offsets with unexpected partition key formats

yashmayya

Thanks for the review Chris!

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java

...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/entities/ConnectorOffset.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/FileOffsetBackingStore.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

C0urante

Thanks Yash, looking great! Just a few more minor comments and then it should be time to start looking at the tests for this.

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/FileOffsetBackingStore.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java

…handling in each branch

…erderRequestHandler::completeRequest; replace usages of HerderRequestHandler::completeOrForwardRequest with HerderRequestHandler::completeRequest for requests that don't involve forwarding

… duplicate partition key processing logic from OffsetBackingStore implementations to OffsetUtils; add unit tests

C0urante

Thanks Yash, looking great! Going to start going over testing changes next; all the comments here are fairly minor.

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java

connect/runtime/src/main/java/org/apache/kafka/connect/util/ConnectUtils.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/OffsetUtils.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/OffsetUtilsTest.java

…der executor thread; other minor refactors

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

connect/runtime/src/main/java/org/apache/kafka/connect/storage/OffsetUtils.java

C0urante

Phew, done with the tests! This is looking really good.

...ct/runtime/src/test/java/org/apache/kafka/connect/integration/OffsetsApiIntegrationTest.java

...t/runtime/src/test/java/org/apache/kafka/connect/integration/MonitorableSourceConnector.java

...ct/runtime/src/test/java/org/apache/kafka/connect/integration/OffsetsApiIntegrationTest.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/FileOffsetBackingStoreTest.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java

…ts; other minor refactors

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/FileOffsetBackingStoreTest.java

connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStoreTest.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java

C0urante · 2023-03-30T15:50:13Z

Looking great! Few small comments left and then this should be good to merge.

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java

…andaloneHerder to avoid concurrent calls to Worker; fix failing unit test in AbstractHerderTest

C0urante

LGTM. Great work, Yash.

C0urante · 2023-04-02T17:11:58Z

FYI I'm going to be taking next week off relaxing on a warm beach somewhere. I don't think we'll have time to get the write/reset API merged in time for the April 12th feature freeze unless someone else can review the PRs for those changes; it may be worth pinging somebody for review on them for visibility.

chia7712 · 2024-05-05T05:29:49Z

connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaOffsetBackingStore.java

-     * @deprecated use {@link #KafkaOffsetBackingStore(Supplier, Supplier)} instead
-     */
-    @Deprecated
-    public KafkaOffsetBackingStore() {


Sorry for making noise on this PR. out of curiosity, should we remove deprecated constructors from KafkaStatusBackingStore and KafkaConfigBackingStore too? not sure whether those internal classes need the deprecation cycle?

@chia7712 I've raised this PR to remove the other two deprecated constructors - #15865 (and also added some context in the PR on why deprecation was introduced rather than removal even though these classes are not in the public API).

…re classes (#15865) - These constructors were deprecated over 3 years ago in KAFKA-10021: Changed Kafka backing stores to use shared admin client to get end offsets and create topics #9780. - While these classes are not a part of Connect's public API, deprecation was still introduced instead of outright removal because they are useful utility classes that might've been used outside of Connect. - The KafkaOffsetBackingStore's deprecated constructor was removed in KAFKA-14785: Connect offset read REST API #13434. - This patch removes the deprecated constructors for KafkaConfigBackingStore and KafkaStatusBackingStore. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

…re classes (apache#15865) - These constructors were deprecated over 3 years ago in KAFKA-10021: Changed Kafka backing stores to use shared admin client to get end offsets and create topics apache#9780. - While these classes are not a part of Connect's public API, deprecation was still introduced instead of outright removal because they are useful utility classes that might've been used outside of Connect. - The KafkaOffsetBackingStore's deprecated constructor was removed in KAFKA-14785: Connect offset read REST API apache#13434. - This patch removes the deprecated constructors for KafkaConfigBackingStore and KafkaStatusBackingStore. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

yashmayya force-pushed the KAFKA-14785-offset-read-api branch from bc6a744 to f54aab6 Compare March 22, 2023 09:16

yashmayya commented Mar 22, 2023

View reviewed changes

yashmayya force-pushed the KAFKA-14785-offset-read-api branch from f54aab6 to 050d80f Compare March 22, 2023 09:37

C0urante added connect kip Requires or implements a KIP labels Mar 22, 2023

KAFKA-14785: Connector offset read REST API

7f2af62

yashmayya force-pushed the KAFKA-14785-offset-read-api branch from 050d80f to 7f2af62 Compare March 23, 2023 13:45

Update trace log in MonitorableSourceConnector

81cae66

C0urante reviewed Mar 24, 2023

View reviewed changes

yashmayya commented Mar 25, 2023

View reviewed changes

C0urante reviewed Mar 27, 2023

View reviewed changes

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java Outdated Show resolved Hide resolved

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved

C0urante reviewed Mar 27, 2023

View reviewed changes

yashmayya added 3 commits March 28, 2023 11:00

Centralize exception handling in Worker::connectorOffsets instead of …

44c0f88

…handling in each branch

Refactor HerderRequestHandler::completeOrForwardRequest to leverage H…

bb68bca

…erderRequestHandler::completeRequest; replace usages of HerderRequestHandler::completeOrForwardRequest with HerderRequestHandler::completeRequest for requests that don't involve forwarding

Add fine grained error messages to partition key processing; refactor…

57dc764

… duplicate partition key processing logic from OffsetBackingStore implementations to OffsetUtils; add unit tests

yashmayya mentioned this pull request Mar 28, 2023

KAFKA-14368: Connect offset write REST API #13465

Merged

3 tasks

C0urante reviewed Mar 28, 2023

View reviewed changes

Run offset get requests on a worker executor thread rather than a her…

4b6c936

…der executor thread; other minor refactors

C0urante reviewed Mar 29, 2023

View reviewed changes

yashmayya added 2 commits March 30, 2023 10:45

Submit request to executor only while fetching source connector offse…

ea01ed1

…ts; other minor refactors

Test improvements

42cd2d3

C0urante reviewed Mar 30, 2023

View reviewed changes

Minor cleanups

42eddd7

C0urante reviewed Mar 31, 2023

View reviewed changes

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

Override AbstractHerder::connectorOffsets in DistributedHerder and St…

844c1fe

…andaloneHerder to avoid concurrent calls to Worker; fix failing unit test in AbstractHerderTest

C0urante approved these changes Apr 2, 2023

View reviewed changes

C0urante merged commit 970dea6 into apache:trunk Apr 2, 2023

yashmayya mentioned this pull request Apr 17, 2023

KAFKA-14876: Document the new 'GET /connectors/{name}/offsets' REST API for Connect #13587

Merged

3 tasks

yashmayya mentioned this pull request Jun 6, 2023

KAFKA-14784: Connect offset reset REST API #13818

Merged

3 tasks

chia7712 reviewed May 5, 2024

View reviewed changes

yashmayya mentioned this pull request May 5, 2024

MINOR: Remove deprecated constructors from Connect's Kafka*BackingStore classes #15865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14785: Connect offset read REST API #13434

KAFKA-14785: Connect offset read REST API #13434

yashmayya commented Mar 22, 2023

C0urante left a comment

yashmayya left a comment

C0urante left a comment

C0urante left a comment

C0urante left a comment

C0urante commented Mar 30, 2023

C0urante left a comment

C0urante commented Apr 2, 2023 •

edited

Loading

chia7712 May 5, 2024

yashmayya May 5, 2024

KAFKA-14785: Connect offset read REST API #13434

KAFKA-14785: Connect offset read REST API #13434

Conversation

yashmayya commented Mar 22, 2023

Committer Checklist (excluded from commit message)

C0urante left a comment

Choose a reason for hiding this comment

yashmayya left a comment

Choose a reason for hiding this comment

C0urante left a comment

Choose a reason for hiding this comment

C0urante left a comment

Choose a reason for hiding this comment

C0urante left a comment

Choose a reason for hiding this comment

C0urante commented Mar 30, 2023

C0urante left a comment

Choose a reason for hiding this comment

C0urante commented Apr 2, 2023 • edited Loading

chia7712 May 5, 2024

Choose a reason for hiding this comment

yashmayya May 5, 2024

Choose a reason for hiding this comment

C0urante commented Apr 2, 2023 •

edited

Loading