You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/06/24 00:25:34 UTC
[GitHub] [kafka] satishbellapu opened a new pull request #8921: Update MirrorConnectorConfig.java
satishbellapu opened a new pull request #8921:
URL: https://github.com/apache/kafka/pull/8921
KAFKA-10160: 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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#discussion_r448872352
##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##########
@@ -67,6 +67,8 @@
protected static final String SYNC_TOPIC_ACLS = "sync.topic.acls";
protected static final String EMIT_HEARTBEATS = "emit.heartbeats";
protected static final String EMIT_CHECKPOINTS = "emit.checkpoints";
+ protected static final String AUTH_OFFSET_RESET = "auto.offset.reset";
+ protected static final String ENABLE_AUTO_COMMIT = "enable.auto.commit";
Review comment:
Can we reuse `ConsumerConfig.ENABLE_AUTO_COMMIT_CONFIG` instead of defining a new constant?
##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##########
@@ -229,8 +235,8 @@ Duration adminTimeout() {
props.putAll(originalsWithPrefix(SOURCE_CLUSTER_PREFIX));
props.keySet().retainAll(MirrorClientConfig.CLIENT_CONFIG_DEF.names());
props.putAll(originalsWithPrefix(CONSUMER_CLIENT_PREFIX));
- props.put("enable.auto.commit", "false");
- props.put("auto.offset.reset", "earliest");
+ props.put(ENABLE_AUTO_COMMIT, "false");
+ props.put(AUTH_OFFSET_RESET, CONSUMER_AUTO_OFFSET_RESET);
Review comment:
If a user sets `somesource->sometarget.consumer.auto.offset.reset=latest` that should be picked up by line 235-237.
So instead of defining a new config on `MirrorConnectorConfig`, could we do something like:
`props.putIfAbsent(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");`
WDYT?
##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##########
@@ -67,6 +67,8 @@
protected static final String SYNC_TOPIC_ACLS = "sync.topic.acls";
protected static final String EMIT_HEARTBEATS = "emit.heartbeats";
protected static final String EMIT_CHECKPOINTS = "emit.checkpoints";
+ protected static final String AUTH_OFFSET_RESET = "auto.offset.reset";
Review comment:
Can we reuse `ConsumerConfig.AUTO_OFFSET_RESET_CONFIG` instead of defining a new constant?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu edited a comment on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu edited a comment on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-648602105
@omkreddy can you review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-650419736
+ @cmccabe @rajinisivaram for review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on a change in pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on a change in pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#discussion_r449727554
##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##########
@@ -229,8 +235,8 @@ Duration adminTimeout() {
props.putAll(originalsWithPrefix(SOURCE_CLUSTER_PREFIX));
props.keySet().retainAll(MirrorClientConfig.CLIENT_CONFIG_DEF.names());
props.putAll(originalsWithPrefix(CONSUMER_CLIENT_PREFIX));
- props.put("enable.auto.commit", "false");
- props.put("auto.offset.reset", "earliest");
+ props.put(ENABLE_AUTO_COMMIT, "false");
+ props.put(AUTH_OFFSET_RESET, CONSUMER_AUTO_OFFSET_RESET);
Review comment:
ACK
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-651512236
@mimaison any comments ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-657451241
ok to test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-649007937
@mjsax can you label appropriately for merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-797420022
@askldjd Good catch, you're right it looks like this was only merged into 2.4. I've reopened [KAFKA-10160](https://issues.apache.org/jira/browse/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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-651658489
@satishbellapu I'll try to take a look Thursday or Friday
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] sameer2800 commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
sameer2800 commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-755307931
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ryannedolan commented on a change in pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
ryannedolan commented on a change in pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#discussion_r444580176
##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##########
@@ -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.";
Review comment:
can leave off "unless specified" -- is redundant with "default".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-648602105
@omkreddy cc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison merged pull request #8921:
URL: https://github.com/apache/kafka/pull/8921
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-653745978
Thanks @satishbellapu for the update, it looks good. Can we add a test too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on a change in pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on a change in pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#discussion_r444586705
##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##########
@@ -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.";
Review comment:
Updated "Consumer Auto offset reset, default to earliest unless specified."
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-659272353
Test failure is unrelated:
- org.apache.kafka.streams.integration.BranchedMultiLevelRepartitionConnectedTopologyTest.testTopologyBuild
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] askldjd commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
askldjd commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-797052226
https://github.com/apache/kafka/pull/8921#issuecomment-755307931
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishbellapu commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
satishbellapu commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-657173935
@mimaison can you review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
mimaison commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-802792953
Actually the fix is in 2.8 and above. It was made in https://github.com/apache/kafka/commit/cf202cb6acf38c64a3e8b9e541673a12ee55eaaa
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] askldjd commented on pull request #8921: KAFKA-10160: Kafka MM2 consumer configuration
Posted by GitBox <gi...@apache.org>.
askldjd commented on pull request #8921:
URL: https://github.com/apache/kafka/pull/8921#issuecomment-797598156
> @askldjd Good catch, you're right it looks like this was only merged into 2.4. I've reopened [KAFKA-10160](https://issues.apache.org/jira/browse/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org