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.

Sign up for GitHub

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

Jump to bottom

KAFKA-14784: Connect offset reset REST API #13818

Merged
merged 7 commits into from Jun 23, 2023
Merged

KAFKA-14784: Connect offset reset REST API #13818

merged 7 commits into from Jun 23, 2023

Conversation

yashmayya
Copy link
Contributor

@yashmayya yashmayya commented Jun 6, 2023 •
edited

Committer Checklist (excluded from commit message)

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

@yashmayya yashmayya added connect kip Requires or implements a KIP labels Jun 6, 2023
@yashmayya
Copy link
Contributor Author

Sink connector offsets alter requests:

ConnectorsResource::alterConnectorOffsets -> AbstractHerder::alterConnectorOffsets -> (Distributed|Standalone)Herder::modifyConnectorOffsets -> Worker::alterConnectorOffsets -> Worker::modifySinkConnectorOffsets -> Worker::alterSinkConnectorOffset

Source connector offsets alter requests:

ConnectorsResource::alterConnectorOffsets -> AbstractHerder::alterConnectorOffsets -> (Distributed|Standalone)Herder::modifyConnectorOffsets -> Worker::alterConnectorOffsets -> Worker::modifySourceConnectorOffsets

Sink connector offsets reset requests:

ConnectorsResource::resetConnectorOffsets -> AbstractHerder::resetConnectorOffsets -> (Distributed|Standalone)Herder::modifyConnectorOffsets -> Worker::resetConnectorOffsets -> Worker::modifySinkConnectorOffsets -> Worker::resetSinkConnectorOffsets

Source connector offsets reset requests:

ConnectorsResource::resetConnectorOffsets -> AbstractHerder::resetConnectorOffsets -> (Distributed|Standalone)Herder::modifyConnectorOffsets -> Worker::resetConnectorOffsets -> Worker::modifySourceConnectorOffsets

The current flows for altering and resetting offsets along with the use of null offsets in multiple places to distinguish between alter and reset offsets requests might seem a little clunky (especially for sink connectors), but I've tried to optimize for code re-use in both the herder implementations as well as the worker.

@yashmayya yashmayya requested a review from C0urante June 6, 2023 12:11
C0urante
C0urante reviewed Jun 7, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

This looks great, thanks Yash! I've made it through all of the functional changes and the integration tests; will review the unit tests in the next pass.

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java Outdated Show resolved Hide resolved
...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java Outdated Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated Show resolved Hide resolved
...ct/runtime/src/test/java/org/apache/kafka/connect/integration/OffsetsApiIntegrationTest.java Outdated Show resolved Hide resolved
...ct/runtime/src/test/java/org/apache/kafka/connect/integration/OffsetsApiIntegrationTest.java Show resolved Hide resolved
@yashmayya yashmayya force-pushed the KAFKA-14784-offset-reset-api branch from 2170332 to 397b622 Compare June 7, 2023 18:08
@yashmayya
Consolidate Worker::alterConnectorOffsets and Worker::resetConnectorO…
355c687 
…ffsets; use a timer to bound total runtime for modifying connector offsets in the Worker; add integration test to verify various invalid offset scenarios while attempting to alter connector offsets; other minor logging and Javadoc improvements
@yashmayya yashmayya force-pushed the KAFKA-14784-offset-reset-api branch from e8fda70 to 355c687 Compare June 9, 2023 09:56
C0urante
C0urante reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

Looking great! Some minor comments and then this should be ready to merge (pending CI build). Thanks Yash!

...ct/runtime/src/test/java/org/apache/kafka/connect/integration/OffsetsApiIntegrationTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java Outdated Show resolved Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java Show resolved Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Show resolved Hide resolved
@yashmayya yashmayya force-pushed the KAFKA-14784-offset-reset-api branch from 7907f2c to cd4c1f2 Compare June 23, 2023 13:00
C0urante
C0urante approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

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

💯 LGTM, thanks Yash!

@C0urante
Copy link
Contributor

Test failures appear unrelated; merging...

@C0urante C0urante merged commit 6e72986 into apache:trunk Jun 23, 2023
1 check failed
@yashmayya yashmayya mentioned this pull request Jun 26, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@C0urante C0urante C0urante approved these changes

Assignees
No one assigned
Labels
connect kip Requires or implements a KIP
Projects
None yet
Milestone
No milestone
2 participants
@yashmayya @C0urante