[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-15 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r505512942



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##
@@ -245,10 +254,17 @@ public MirrorClientConfig clientConfig(String cluster) {
 Map strings = originalsStrings();
 strings.keySet().removeIf(x -> !x.startsWith(prefix));
 return strings;
-} 
+}
+
+static Map prefixForGeneralAttrs(String prefix, 
Map props) {

Review comment:
   Indeed. Thanks!





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-14 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r503489280



##
File path: 
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorMakerConfigTest.java
##
@@ -52,10 +52,10 @@ public void testClusterConfigProperties() {
 "replication.factor", "4"));
 Map connectorProps = 
mirrorConfig.connectorBaseConfig(new SourceAndTarget("a", "b"),
 MirrorSourceConnector.class);
-assertEquals("source.cluster.bootstrap.servers is set",
-"servers-one", 
connectorProps.get("source.cluster.bootstrap.servers"));
-assertEquals("target.cluster.bootstrap.servers is set",
-"servers-two", 
connectorProps.get("target.cluster.bootstrap.servers"));
+assertEquals("source.bootstrap.servers is set",

Review comment:
   I think I have achieved what we want. Explicitly setting "source." 
prefix for props starting with consumer|producer|admin and setting 
"source.cluster." otherwise. All tests passed. The code runs fine on my infra.
   
   @mimaison please take a look whenever you get some time :)





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-12 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r503489280



##
File path: 
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorMakerConfigTest.java
##
@@ -52,10 +52,10 @@ public void testClusterConfigProperties() {
 "replication.factor", "4"));
 Map connectorProps = 
mirrorConfig.connectorBaseConfig(new SourceAndTarget("a", "b"),
 MirrorSourceConnector.class);
-assertEquals("source.cluster.bootstrap.servers is set",
-"servers-one", 
connectorProps.get("source.cluster.bootstrap.servers"));
-assertEquals("target.cluster.bootstrap.servers is set",
-"servers-two", 
connectorProps.get("target.cluster.bootstrap.servers"));
+assertEquals("source.bootstrap.servers is set",

Review comment:
   I think I have achieved what we want. Explicitly setting "source." 
prefix for props starting with consumer|producer|admin and setting 
"source.cluster." otherwise. All tests passed. The code runs fine on my infra.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-09 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r502351930



##
File path: 
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorMakerConfigTest.java
##
@@ -52,10 +52,10 @@ public void testClusterConfigProperties() {
 "replication.factor", "4"));
 Map connectorProps = 
mirrorConfig.connectorBaseConfig(new SourceAndTarget("a", "b"),
 MirrorSourceConnector.class);
-assertEquals("source.cluster.bootstrap.servers is set",
-"servers-one", 
connectorProps.get("source.cluster.bootstrap.servers"));
-assertEquals("target.cluster.bootstrap.servers is set",
-"servers-two", 
connectorProps.get("target.cluster.bootstrap.servers"));
+assertEquals("source.bootstrap.servers is set",

Review comment:
   That will require more changes. This does not break for the mirrorMaker 
driver main config. I guess it does for on Connect though 





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-09 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r502351930



##
File path: 
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorMakerConfigTest.java
##
@@ -52,10 +52,10 @@ public void testClusterConfigProperties() {
 "replication.factor", "4"));
 Map connectorProps = 
mirrorConfig.connectorBaseConfig(new SourceAndTarget("a", "b"),
 MirrorSourceConnector.class);
-assertEquals("source.cluster.bootstrap.servers is set",
-"servers-one", 
connectorProps.get("source.cluster.bootstrap.servers"));
-assertEquals("target.cluster.bootstrap.servers is set",
-"servers-two", 
connectorProps.get("target.cluster.bootstrap.servers"));
+assertEquals("source.bootstrap.servers is set",

Review comment:
   That will require more changes. This does not break for the mirrorMaker 
driver main config. I guess it does for on Connect though 





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-07 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r501326680



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I think the new code does what we want. Also tested a bin from this on 
my infra and it is working fine. Let me know if this works for you.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-06 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r500569567



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   臘 got it. I misunderstood it. Let me give it a try.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-06 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r500566839



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   For users running on a Connect cluster, that format is not supported 
right now:
   ```
   source.producer.some-producer-setting: 123
   ```
   
   The only supported is (unless I'm missing something):
   ```
   "producer.some-producer-setting": 123
   ```





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-06 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r500464804



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   @mimaison just a friendly ping.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-02 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r498971069



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   > Maybe not including cluster for all the client specific settings would 
be slightly better, WDYT?
   
   Somehow I missed this. I do not have a strong opinion on that but I do like 
the idea on normalizing it. But changing that will still break existing users 
though. How do you want to proceed?





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-10-01 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r498299493



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   For connect, you can still use (or the one you've shared).
   ```
   "source.cluster.bootstrap.servers": "localhost:9092",
   "source.cluster.security.protocol": "SASL_SSL",
   "producer.some-producer-setting": 123
   "consumer.some-consumer-setting": 123
   "source.admin.some-admin-setting": 123
   ```
   
   Yes, we could normalize that but I'm not touching admin config in this PR.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-30 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r497079320



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I have put a new fix that should handle both cases. Details on why I 
think this is a reasonable fix:
   * There's no change on was is defined 
[here](https://github.com/apache/kafka/tree/trunk/connect/mirror#producer--consumer--admin-config-used-by-mm2).
 The properties that starts with `producer.` or `consumer.` are still ignored 
in mm2 dedicated mode.
   * MM2 needs to differentiate the properties for the source cluster and we 
can't just remove the `source.cluster` prefix.
   * For running the mirror connectors on a Connect cluster, the only 
"drawback" is that now you can provide an override with 
`source.cluster.consumer` or `source.cluster.producer`.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-30 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r497079320



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I have put a new fix that should handle both cases. Details on why I 
think this is a reasonable fix:
   * There's no change on what is defined 
[here](https://github.com/apache/kafka/tree/trunk/connect/mirror#producer--consumer--admin-config-used-by-mm2).
 The properties that starts with `producer.` or `consumer.` are still ignored 
in mm2 dedicated mode.
   * MM2 needs to differentiate the properties for the source cluster and we 
can't just remove the `source.cluster` prefix.
   * For running the mirror connectors on a Connect cluster, the only 
"drawback" is that now you can provide an override with 
`source.cluster.consumer` or `source.cluster.producer`.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-30 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r497079320



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I have put a new fix that should handle both cases. Details on why I 
think this is a good fix:
   * There's no change on was is defined 
[here](https://github.com/apache/kafka/tree/trunk/connect/mirror#producer--consumer--admin-config-used-by-mm2).
 The properties that starts with `producer.` or `consumer.` are still ignored 
in mm2 dedicated mode.
   * MM2 needs to differentiate the properties for the source cluster and we 
can't just remove the `source.cluster` prefix.
   * For running the mirror connectors on a Connect cluster, the only 
"drawback" is that now you can provide an override with 
`source.cluster.consumer` or `source.cluster.producer`.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-29 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r497079320



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I have put a new fix that should handle both cases.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-29 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r495871017



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I'm not sure if I follow it. ~This prefix "source_cluster" is not user 
specified. It is prefixed somewhere else by mm2 (I can get the lines if you 
want later on).~ The config specified 
[here](https://github.com/apache/kafka/tree/trunk/connect/mirror#producer--consumer--admin-config-used-by-mm2)
 will actually be honored. It does not imply changes on the MirrorMaker config 
side at least. Are you talking about the case of running MirrorMaker in a 
connect cluster rather than as a dedicated cluster? I didn't check that one 
TBH. Maybe @ryannedolan has more insight on this one?





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-28 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r495995224



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   Gotcha. I just peek'd at it again and you're right. It seems the proper 
fix would be to fix the way mm2 populates the MirrorConnectorConfig to avoid 
this change. WDYT @ryannedolan ? Not sure how involved this change would be.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-28 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r495995224



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   Gotcha. I just peek at it again and you're right. It seems the proper 
fix would be to fix the way mm2 populates the MirrorConnectorConfig to avoid 
this change. WDYT @ryannedolan ?





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-28 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r495995224



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   Gotcha. I just peek at it again and you're right. It seems the proper 
fix would be to fix the way mm2 populates the MirrorConnectorConfig to avoid 
this change. WDYT @ryannedolan ? Not sure how involved this change would be.





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:
us...@infra.apache.org

[GitHub] [kafka] scanterog commented on a change in pull request #9313: [mm2] Fix consumer/producer properties override

2020-09-28 Thread GitBox 


scanterog commented on a change in pull request #9313:
URL: https://github.com/apache/kafka/pull/9313#discussion_r495871017



##
File path: 
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java
##
@@ -199,8 +199,8 @@
 
 protected static final String SOURCE_CLUSTER_PREFIX = 
MirrorMakerConfig.SOURCE_CLUSTER_PREFIX;
 protected static final String TARGET_CLUSTER_PREFIX = 
MirrorMakerConfig.TARGET_CLUSTER_PREFIX;
-protected static final String PRODUCER_CLIENT_PREFIX = "producer.";
-protected static final String CONSUMER_CLIENT_PREFIX = "consumer.";
+protected static final String PRODUCER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "producer.";
+protected static final String CONSUMER_CLIENT_PREFIX = 
SOURCE_CLUSTER_PREFIX + "consumer.";

Review comment:
   I'm not sure if I follow it. This prefix "source_cluster" is not user 
specified. It is prefixed somewhere else by mm2 (I can get the lines if you 
want later on). The config specified 
[here](https://github.com/apache/kafka/tree/trunk/connect/mirror#producer--consumer--admin-config-used-by-mm2)
 will actually be honored. It does not imply changes on the MirrorMaker config 
side at least. Are you talking about the case of running MirrorMaker in a 
connect cluster rather than as a dedicated cluster? I didn't check that one 
TBH. Maybe @ryannedolan has more insight on this one?





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:
us...@infra.apache.org