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-10160: Kafka MM2 consumer configuration #8921

Merged
merged 4 commits into from
Jul 16, 2020
Merged

KAFKA-10160: Kafka MM2 consumer configuration #8921

merged 4 commits into from
Jul 16, 2020

Conversation

satishbellapu
Copy link

@satishbellapu satishbellapu commented Jun 24, 2020 •
edited
Loading

Removed hardcoded auto.offset.reset in MM2 consumer configuration, retained default as earliest unless specified.

Committer Checklist (excluded from commit message)

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

@satishbellapu
Update MirrorConnectorConfig.java
75b1c77 
KAFKA-10160: Removed hardcoded auto.offset.reset in MM2 consumer configuration, defaults to earliest unless specified.
@satishbellapu satishbellapu changed the title Update MirrorConnectorConfig.java KAFKA-10160: Removed hardcoded auto.offset.reset in MM2 consumer configuration, retained default as earliest unless specified. Jun 24, 2020
@satishbellapu satishbellapu changed the title KAFKA-10160: Removed hardcoded auto.offset.reset in MM2 consumer configuration, retained default as earliest unless specified. KAFKA-10160: Kafka MM2 consumer configuration Jun 24, 2020
ryannedolan
ryannedolan approved these changes Jun 24, 2020
View reviewed changes
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java Outdated
@@ -123,6 +125,10 @@
private static final String CONSUMER_POLL_TIMEOUT_MILLIS_DOC = "Timeout when polling source cluster.";
public static final long CONSUMER_POLL_TIMEOUT_MILLIS_DEFAULT = 1000L;

public static final String CONSUMER_AUTO_OFFSET_RESET = "consumer.auto.offset.reset";
private static final String CONSUMER_AUTO_OFFSET_RESET_DOC = "Consumer Auto offset reset, defaults to earliest unless specify.";
Copy link
Contributor

Choose a reason for hiding this comment

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

can leave off "unless specified" -- is redundant with "default".

Copy link
Author

Choose a reason for hiding this comment

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

Updated "Consumer Auto offset reset, default to earliest unless specified."

@satishbellapu
Copy link
Author

satishbellapu commented Jun 24, 2020 •
edited
Loading

@omkreddy can you review?

@satishbellapu
Copy link
Author

@mjsax can you label appropriately for merge.

@satishbellapu
Copy link
Author

@omkreddy omkreddy requested a review from mimaison June 27, 2020 10:11
@satishbellapu
Copy link
Author

@mimaison any comments ?

@mimaison
Copy link
Member

@satishbellapu I'll try to take a look Thursday or Friday

@mimaison
Copy link
Member

mimaison commented Jul 4, 2020

Thanks @satishbellapu for the update, it looks good. Can we add a test too?

@satishbellapu
Copy link
Author

@mimaison can you review?

@mimaison
Copy link
Member

ok to test

mimaison
mimaison approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@mimaison mimaison 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 for the PR

@mimaison
Copy link
Member

Test failure is unrelated:

  • org.apache.kafka.streams.integration.BranchedMultiLevelRepartitionConnectedTopologyTest.testTopologyBuild

@mimaison mimaison merged commit 2eb10ec into apache:2.4 Jul 16, 2020
@sameer2800
Copy link

@satishbellapu this fix isn't present in the latest version specifically 2.7. I think you have to raise a PR to trunk to make it available for all the future versions.

@askldjd
Copy link

askldjd commented Mar 11, 2021

#8921 (comment)

I am running into this issue right now. It looks like this PR never got merged to trunk. @satishbellapu, are you able to do this?

@mimaison
Copy link
Member

@askldjd Good catch, you're right it looks like this was only merged into 2.4. I've reopened KAFKA-10160.
Let's see if @satishbellapu has time to open a PR in the next few days. Otherwise I'll port this change next week

@askldjd
Copy link

askldjd commented Mar 12, 2021

@askldjd Good catch, you're right it looks like this was only merged into 2.4. I've reopened KAFKA-10160.
Let's see if @satishbellapu has time to open a PR in the next few days. Otherwise I'll port this change next week

Thanks @mimaison. Really appreciate it.

@mimaison
Copy link
Member

Actually the fix is in 2.8 and above. It was made in cf202cb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimaison mimaison mimaison approved these changes

@ryannedolan ryannedolan ryannedolan approved these changes

@adamkeyser adamkeyser adamkeyser approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@satishbellapu @mimaison @sameer2800 @askldjd @ryannedolan @adamkeyser