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/11/12 15:33:45 UTC

[GitHub] [kafka] twobeeb opened a new pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

twobeeb opened a new pull request #9589:
URL: https://github.com/apache/kafka/pull/9589


   As described in JIRA KAFKA-10710, 
   Running mirror maker 2 with multiple clusters defined in the clusters properties creates a herder for all the possible combinations even if the "link" are not wanted. 
   If my use-case, most of topics must be replicated from replica_CENTRAL to replica_<other>.
   Example config file
   ````
   clusters = replica_CENTRAL, replica_OLS, replica_HBG # and lots more
   # cluster connection informations
   replica_CENTRAL.bootstrap.servers = ...
   replica_OLS.bootstrap.servers = ...
   replica_HBG.bootstrap.servers = ...
   ...
   
   replica_CENTRAL->replica_OLS.enabled = true
   replica_CENTRAL->replica_OLS.topics = _schemas
   replica_CENTRAL->replica_HBG.enabled = true
   replica_CENTRAL->replica_HBG.topics = _schemas
   ...
   
   ````
   This leads to an incredible amount of connections, file descriptors and configuration topics created in every target clusters that are not required for this use-case.
   The number of opened file descriptors is more than 10 000 for only one topic replicated from CENTRAL cluster to 10 other clusters
   
   The following commit is a raw suggestion of what should be done to limit the number of herders created on startup.
   
   ### 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] skaundinya15 commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
skaundinya15 commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r564909811



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       Question @twobeeb and @ryannedolan about this: is there a use case for which a cluster pair would be disabled yet its heartbeats enabled? I'm wondering because in the case someone disabled the cluster pair but didn't think to disable the heartbeats (since it defaults to true), these herders would still be created, but I'm not sure if it would be useful to have them created since there is no replication going on. Would it make more sense to just not create a herder if the cluster pair was disabled (instead of checking the heartbeats as well)? If not can we make sure to add documentation about this behavior?




----------------------------------------------------------------
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] twobeeb edited a comment on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb edited a comment on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728593310


   @ryannedolan @hachikuji 
   If I understand correctly, when setting up a link A->B.enabled=true (with defaults settings regarding heartbeat), it creates a topic heartbeat and produce beats on cluster B
   
   I figured it was the other way around, so that the topic could be picked up by replication process (A->B), leading to a replicated topic A.heartbeat on cluster B (which we monitor actually). 
   
   Looking into my configuration, I now understand that I was lucky because I have one link going up back up ``replica_OLS->replica_CENTRAL`` which is now the single emitter of beats (which are then replicated in every other cluster)
   
   Reading the KIP led me to interpret that the production of heartbeat would be done within the same herder (the source one)  : 
   > Internal Topics
   > MM2 emits a heartbeat topic in each source cluster, which is replicated to demonstrate connectivity through the connectors.


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-766978261


   Hi @ryannedolan @hachikuji 
   
   I'd like a definitive direction from you on this subject. 
   
   Once again, I reiterate on my personal preference as expressed previously by @ryannedolan :
   > IMO, we'd ideally skip creating the A->B herder whenever A->B.emit.heartbeats.enabled=false (defaults to true) and A->B.enabled=false (defaults to false). A top-level emit.heartbeats.enabled=false would then disable heartbeats altogether, which would trivially eliminate the extra herders. N.B. this would just be an optimization and wouldn't required a KIP, IMO.
   
   Please advise.
   
   Julien


----------------------------------------------------------------
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] hachikuji merged pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9589:
URL: https://github.com/apache/kafka/pull/9589


   


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-736079387


   @ryannedolan 
   I have a preference for your first suggestion (the one for which there is currently a code proposition) because it doesn't alter the behavior of MM2 in any way for existing users.  
   
   > IMO, we'd ideally skip creating the A->B herder whenever A->B.emit.heartbeats.enabled=false (defaults to true) and A->B.enabled=false (defaults to false). A top-level emit.heartbeats.enabled=false would then disable heartbeats altogether, which would trivially eliminate the extra herders. N.B. this would just be an optimization and wouldn't required a KIP, IMO.
   
   That being said, I'm also fine with your latest proposition. It fits perfectly to my personnal use case because topics will be replicated local to central as well as central to local in the near future ; all instanciated herders will then transmit data (and not just beats).
   
   Could you confirm and/or elaborate on the behavior you want to see when playing with ``emit.heartbeats.enabled``  ?
   
   I guess that'd be good test cases : 
   
   **Simple A to B**
   ````
   A->B.enabled=true
   ````
   Expected : 2 herders
     
   **A to B and A to C**
   ```` 
   clusters=A, B, C
   A->B.enabled=true
   A->C.enabled=true
   ````
    Expected : 4 herders (AB BA AC CA)
   
   **Disabled heartbeats**
   ```` 
   clusters=A, B, C
   A->B.enabled=true
   A->B.emit.heartbeats.enabled=false
   B->A.emit.heartbeats.enabled=false
   A->C.enabled=true
   A->C.emit.heartbeats.enabled=false
   C->A.emit.heartbeats.enabled=false
   ````
   Expected : 4 herders (2 of which are unused, but still much better than N*N-1)


----------------------------------------------------------------
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] twobeeb commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r565704218



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       Thanks for your review @skaundinya15.
   I'm having a hard time phrasing this properly, suggestions would be welcome.
   Is this comment proposition aligned with what you had in mind ?
   ```suggestion
                       // By default, all source->target Herder combinations are created even if `x->y.enabled=false`
                       // Unless `emit.heartbeats.enabled=false` or `x->y.emit.heartbeats.enabled=false`
                       // Reason for this behavior: for a given replication flow A->B with heartbeats, 2 herders are required :
                       // B->A for the MirrorHeartbeatConnector (emits heartbeats into A)
                       // A->B for the MirrorSourceConnector (actual replication flow)
                       if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {
   ```




----------------------------------------------------------------
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] twobeeb commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r565109219



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       That's actually the current behavior, all herders are created and the beats are emitted from the "opposite" herder  
   
   If you have a replication flow from A to B **and you want heartbeats**, you need 2 herders :  
   - A->B for the MirrorSourceConnector 
   - B->A for the MirrorHeartbeatConnector 
   
   The MirrorHeartbeatConnector on B->A emits beats into topic heartbeats on cluster A.
   The MirrorSourceConnector on A->B then replicates whichever topic is configured as well as heartbeats.
   
   All of this is fine with 2-3 clusters. It starts to make less sense when using 10+ clusters.
   
   Because this is the current implementation, we decided to keep it as-is so as to not cause change in behavior, and add a new top-level property "emit.heartbeats.enabled" which defaults to "true".
   
   Existing users will not see any change and if they depend on these "opposites" herders for their monitoring, it will still work.  
   New users with more complex use case can change this property and fine tune their heartbeat generation.




----------------------------------------------------------------
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] twobeeb edited a comment on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb edited a comment on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728593310


   @ryannedolan @hachikuji 
   If I understand correctly, when setting up a link A->B.enabled=true (with defaults settings regarding heartbeat), it creates a topic heartbeat which produces beats on B
   
   I figured it was the other way around, so that the topic could be picked up by replication process, leading to a replicated topic B.heartbeat (which we monitor actually). 
   
   Looking into my configuration, I now understand that I was lucky because I have one link going up back up ``replica_OLS->replica_CENTRAL`` which is now the single emitter of beats (which are then replicated in every other cluster)
   
   Reading the KIP led me to interpret that the production of heartbeat would be done within the same herder (the source one)  : 
   > Internal Topics
   > MM2 emits a heartbeat topic in each source cluster, which is replicated to demonstrate connectivity through the connectors.


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728339571


   Thank you for your help @hachikuji. I agree with your analysis and it kind of makes sense for most use-cases with 2-3 clusters as described in the original KIP.
   I also understand that the original intent was that the ``clusterPairs`` variable would not grow exponential thanks to the command line parameter ``--clusters`` which is target-based (not source-based).
    
   But this parameter doesn't help the business case we are implementing which can be better viewed as one "central cluster" and multiple "local clusters":
   - Some topics (_schema is a perfect example) must be replicated down to every local cluster, 
   - and some "local" topics will be replicated up to the central cluster. 
   
   As stated in the KAFKA-10710, I cherry picked this commit in 2.5.2 and we are now running this build of MirrorMaker in production, because we can't ramp up deployment with current code as it stands.


----------------------------------------------------------------
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 pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
ryannedolan commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-731307334


   > Looking into my configuration, I now understand that I was lucky because I have one link going up back up replica_OLS->replica_CENTRAL which is now the single emitter of beats (which are then replicated in every other cluster)
   
   Ah, that's lucky indeed. If you sever that link, I suppose your heartbeats will stop flowing across the entire topology. Also, if you look at the heartbeat message content, you'll probably find that all heartbeats are originating from that same cluster.
   
   Sounds like disabling heartbeats altogether isn't what you want either. So we may need a way to emit heartbeats to source clusters without spinning up N*(N-1) herders just to do so.
   
   What if we created N additional herders just for heartbeats to each cluster? Say you had 20 clusters and only one flow enabled (A->B). In that case you could have 21 herders total: one for the A->B flow and 20 for per-cluster heartbeats. That's at least better than 20*19=380 herders. This raises the question of what "source" to associated with heartbeats, but seems like a solvable problem.
   
   Another idea is to have herders emit heartbeats _upstream_ within MirrorSourceConnector. In that case, the A->B herder would target B but, paradoxically, emit heartbeats to A. Hard to say if that would break anything, but as @twobeeb  points out, this may be a reasonable interpretation of the documentation.


----------------------------------------------------------------
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] hachikuji commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-768756102


   @twobeeb Before I merge, would you mind updating the PR description? Also, I will leave it to you to add the doc suggestion from @skaundinya15. 


----------------------------------------------------------------
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] twobeeb edited a comment on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb edited a comment on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-726846099


   After fixing the issue : 
   ````
   [2020-11-13 15:53:53,935] INFO creating herder for replica_CENTRAL->replica_ZAR (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:53:58,597] INFO creating herder for replica_CENTRAL->replica_UGO (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:01,180] INFO creating herder for replica_CENTRAL->replica_HBG (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:03,656] INFO creating herder for replica_CENTRAL->replica_OLS (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:06,097] INFO creating herder for replica_CENTRAL->replica_UMO (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:08,186] INFO creating herder for replica_OLS->replica_CENTRAL (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:09,774] INFO creating herder for replica_CENTRAL->replica_CNO (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:12,253] INFO creating herder for replica_CENTRAL->replica_TST (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:14,407] INFO creating herder for replica_CENTRAL->replica_VLD (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:16,599] INFO creating herder for replica_CENTRAL->replica_ARA (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:19,005] INFO creating herder for replica_CENTRAL->replica_VIT (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:21,789] INFO Kafka MirrorMaker starting with 11 herders. (org.apache.kafka.connect.mirror.MirrorMaker)
   ````


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728593310


   @ryannedolan @hachikuji 
   If I understand correctly, when setting up a link A->B.enabled=true (with defaults settings regarding heartbeat), it creates a topic heartbeat which produces beats on B
   
   I figured it was the other way around, so that the topic could be picked up by replication process, leading to a replicated topic B.heartbeat (which we monitor actually). 
   
   Looking into my configuration, I now understand that I was lucky because I have one link going up back up ``replica_OLS->replica_CENTRAL`` which is now the single emitter of beats (which are then replicated in every other cluster)
   
   Reading the KIP lead me to interpret that the production of heartbeat would be done within the same herder (the source one)  : 
   > Internal Topics
   > MM2 emits a heartbeat topic in each source cluster, which is replicated to demonstrate connectivity through the connectors.
   
   Wouldn't it make more sense to produce the beats from the same "side" of the replication ?


----------------------------------------------------------------
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] twobeeb commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r565109219



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       That's actually the current behavior, all herders are created and the beats are emitted from the "opposite" herder:  
   
   If you have a replication flow from A to B **and you want heartbeats**, you need 2 herders :  
   - A->B for the MirrorSourceConnector 
   - B->A for the MirrorHeartbeatConnector 
   
   The MirrorHeartbeatConnector on B->A emits beats into topic heartbeats on cluster A.
   The MirrorSourceConnector on A->B then replicated whichever topic is configured as well as heartbeats.
   
   All of this is fine with 2-3 clusters. It starts to make less sense when using 10+ clusters.
   
   Because this is the current implementation, we decided to keep it as-is so as to not cause change in behavior, and add a new top-level property "emit.heartbeats.enabled" which defaults to "true".
   
   Existing users will not see any change and if they depend on these "opposites" herders for their monitoring, it will still work.  
   New users with more complex use case can change this property and fine tune their heartbeat generation.




----------------------------------------------------------------
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 pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
ryannedolan commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728354718


   > The code seems to explicitly allow the connector to be created even when the link is disabled.
   > @ryannedolan maybe you could clarify?
   
   My intention was to ensure that MirrorHeartbeatConnector always runs, even when a link/flow is not otherwise needed. This is because MirrorHeartbeatConnector is most useful when it emits to all clusters, not just those clusters targeted by some flow.
   
   For example, in a two-cluster environment with only A->B replicated, it is nice to have heartbeats emitted to A s.t. they get replicated to B. Without a Herder targeting A, there can be no MirrorHeartbeatConnector emitting heartbeats there, and B will see no heartbeats from A.
   
   I know that some vendors/providers use heartbeats in this way, e.g. for discovering which flows are active and healthy. And I know that some vendors/providers don't use heartbeats at all, or use something else to send them (instead of MirrorHeartbeatConnector). Hard to say whether anything would break if we nixed these extra herders without addressing the heartbeats that would go missing.
   
   IMO, we'd ideally skip creating the A->B herder whenever A->B.emit.heartbeats.enabled=false (defaults to true) and A->B.enabled=false (defaults to false). A top-level emit.heartbeats.enabled=false would then disable heartbeats altogether, which would trivially eliminate the extra herders. N.B. this would just be an optimization and wouldn't required a KIP, IMO.
   
   Ryanne
    


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-768920853


   @hachikuji
   I included the comment from @skaundinya15 and changed the PR description to reflect the actual design choice.
   
   Let me know if that's ok 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:
users@infra.apache.org



[GitHub] [kafka] ijuma commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r566540886



##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorMakerConfigTest.java
##########
@@ -256,6 +257,80 @@ public void testWorkerConfigs() {
             "secret2", bProps.get("producer.ssl.key.password"));
     }
 
+    @Test
+    public void testClusterPairsWithDefaultSettings() {
+        MirrorMakerConfig mirrorConfig = new MirrorMakerConfig(makeProps(
+                "clusters", "a, b, c"));
+        // implicit configuration associated
+        // a->b.enabled=false
+        // a->b.emit.heartbeat.enabled=true
+        // a->c.enabled=false
+        // a->c.emit.heartbeat.enabled=true
+        // b->a.enabled=false
+        // b->a.emit.heartbeat.enabled=true
+        // b->c.enabled=false
+        // b->c.emit.heartbeat.enabled=true
+        // c->a.enabled=false
+        // c->a.emit.heartbeat.enabled=true
+        // c->b.enabled=false
+        // c->b.emit.heartbeat.enabled=true
+        List<SourceAndTarget> clusterPairs = mirrorConfig.clusterPairs();
+        assertEquals("clusterPairs count should match all combinations count",

Review comment:
       I fixed the build via https://github.com/apache/kafka/commit/bf4afae8f53471ab6403cbbfcd2c4e427bdd4568




----------------------------------------------------------------
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] twobeeb edited a comment on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb edited a comment on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728593310


   @ryannedolan @hachikuji 
   If I understand correctly, when setting up a link A->B.enabled=true (with defaults settings regarding heartbeat), it creates a topic heartbeat which produces beats on B
   
   I figured it was the other way around, so that the topic could be picked up by replication process, leading to a replicated topic B.heartbeat (which we monitor actually). 
   
   Looking into my configuration, I now understand that I was lucky because I have one link going up back up ``replica_OLS->replica_CENTRAL`` which is now the single emitter of beats (which are then replicated in every other cluster)
   
   Reading the KIP led me to interpret that the production of heartbeat would be done within the same herder (the source one)  : 
   > Internal Topics
   > MM2 emits a heartbeat topic in each source cluster, which is replicated to demonstrate connectivity through the connectors.
   
   Wouldn't it make more sense to produce the beats from the same "side" of the replication ?


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-766978261


   Hi @ryannedolan @hachikuji 
   
   I'd like a definitive direction from you on this subject. 
   
   Once again, I reiterate on my personal preference as expressed previously by @ryannedolan :
   > IMO, we'd ideally skip creating the A->B herder whenever A->B.emit.heartbeats.enabled=false (defaults to true) and A->B.enabled=false (defaults to false). A top-level emit.heartbeats.enabled=false would then disable heartbeats altogether, which would trivially eliminate the extra herders. N.B. this would just be an optimization and wouldn't required a KIP, IMO.
   
   Please advise.
   
   Julien


----------------------------------------------------------------
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] skaundinya15 commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
skaundinya15 commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r565772802



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       ```suggestion
                       // By default, all source->target Herder combinations are created even if `x->y.enabled=false`
                       // Unless `emit.heartbeats.enabled=false` or `x->y.emit.heartbeats.enabled=false`
                       // Reason for this behavior: for a given replication flow A->B with heartbeats, 2 herders are required :
                       // B->A for the MirrorHeartbeatConnector (emits heartbeats into A for monitoring replication health)
                       // A->B for the MirrorSourceConnector (actual replication flow)
                       if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {
   ```
   Looks good to me, just had a small tweak for the `B->A` comment. 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:
users@infra.apache.org



[GitHub] [kafka] ryannedolan commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
ryannedolan commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-767570251


   > I have a preference for your first suggestion ... because it doesn't alter the behavior of MM2 in any way for existing users.
   
   Good point, and also it doesn't conflict with the other ideas if we want to try something later. Works for me.


----------------------------------------------------------------
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] hachikuji commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728294205


   Thanks for the patch. The proposed fix makes sense. I am only trying to confirm that this is a defect and not intended. The code seems to explicitly allow the connector to be created even when the link is disabled. Perhaps the cost of a disabled herder was not understood.
   
   @ryannedolan maybe you could clarify?


----------------------------------------------------------------
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] twobeeb commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r565704218



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       @skaundinya15 Thanks for your review.
   I'm having a hard time phrasing this properly, suggestions would be welcome.
   Is this comment proposition aligned with what you had in mind ?
   ```suggestion
                       // By default, all source->target Herder combinations are created even if `x->y.enabled=false`
                       // Unless `emit.heartbeats.enabled=false` or `x->y.emit.heartbeats.enabled=false`
                       // Reason for this behavior: for a given replication flow A->B with heartbeats, 2 herders are required :
                       // B->A for the MirrorHeartbeatConnector (emits heartbeats into A)
                       // A->B for the MirrorSourceConnector (actual replication flow)
                       if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {
   ```




----------------------------------------------------------------
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] skaundinya15 commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
skaundinya15 commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r565656131



##########
File path: connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
##########
@@ -89,11 +89,25 @@ public MirrorMakerConfig(Map<?, ?> props) {
     public List<SourceAndTarget> clusterPairs() {
         List<SourceAndTarget> pairs = new ArrayList<>();
         Set<String> clusters = clusters();
+        Map<String, String> originalStrings = originalsStrings();
+        boolean globalHeartbeatsEnabled = MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED_DEFAULT;
+        if (originalStrings.containsKey(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+            globalHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+        }
+
         for (String source : clusters) {
             for (String target : clusters) {
-                SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
                 if (!source.equals(target)) {
-                    pairs.add(sourceAndTarget);
+                    String clusterPairConfigPrefix = source + "->" + target + ".";
+                    boolean clusterPairEnabled = Boolean.valueOf(originalStrings.getOrDefault(clusterPairConfigPrefix + "enabled", "false"));
+                    boolean clusterPairHeartbeatsEnabled = globalHeartbeatsEnabled;
+                    if (originalStrings.containsKey(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED)) {
+                        clusterPairHeartbeatsEnabled = Boolean.valueOf(originalStrings.get(clusterPairConfigPrefix + MirrorConnectorConfig.EMIT_HEARTBEATS_ENABLED));
+                    }
+
+                    if (clusterPairEnabled || clusterPairHeartbeatsEnabled) {

Review comment:
       Thanks for the explanation @twobeeb, this makes sense. It would be good to add some comments explaining this in the code as this isn't immediately obvious. Other than that it looks good to me overall.




----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-729529865


   @hachikuji ready for review, I believe failed CI is unrelated


----------------------------------------------------------------
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 pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
ryannedolan commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-735039590


   What if we just run herders in both directions (A->B and B->A) whenever replication is enabled in one or both directions? That would require very minimal code changes, and only in the driver, while yielding the results you are expecting. Namely, we'd still see heartbeats on every _source_ cluster, and we'd still see heartbeats get replicated along each replication flow, but we would eliminate a ton of extraneous herders in the scenario you've illustrated.
   
   Specifically, if we see A->B.enabled, we run MirrorSourceConnector and MirrorCeckpointConnector in herder A->B, and we run MirrorHeartbeatConnector in herder B->A. But if neither A->B nor B->A is enabled, then we don't need to create either herder at all.
   
   This would change the behavior of the MM2 driver very slightly, since we'd no longer emit heartbeats to clusters that aren't part of any flow. But that doesn't really seem like a breaking change to me. "If a tree falls in the woods" and all that.
   
   What do you think @twobeeb ?


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-726846099


   After fixing the issue : 
   ````
   [root@lnx001385 log]# cat kafka-mirror-maker.log | grep herder
   [2020-11-13 15:53:53,935] INFO creating herder for replica_CENTRAL->replica_ZAR (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:53:58,597] INFO creating herder for replica_CENTRAL->replica_UGO (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:01,180] INFO creating herder for replica_CENTRAL->replica_HBG (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:03,656] INFO creating herder for replica_CENTRAL->replica_OLS (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:06,097] INFO creating herder for replica_CENTRAL->replica_UMO (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:08,186] INFO creating herder for replica_OLS->replica_CENTRAL (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:09,774] INFO creating herder for replica_CENTRAL->replica_CNO (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:12,253] INFO creating herder for replica_CENTRAL->replica_TST (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:14,407] INFO creating herder for replica_CENTRAL->replica_VLD (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:16,599] INFO creating herder for replica_CENTRAL->replica_ARA (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:19,005] INFO creating herder for replica_CENTRAL->replica_VIT (org.apache.kafka.connect.mirror.MirrorMaker)
   [2020-11-13 15:54:21,789] INFO Kafka MirrorMaker starting with 11 herders. (org.apache.kafka.connect.mirror.MirrorMaker)
   ````


----------------------------------------------------------------
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] twobeeb commented on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb commented on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-733687425


   @ryannedolan I'm fine with either solution you suggest. What's the next step here ? I'm happy to contribute but I need your help on which direction we should take.


----------------------------------------------------------------
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] twobeeb edited a comment on pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
twobeeb edited a comment on pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#issuecomment-728593310


   @ryannedolan @hachikuji 
   If I understand correctly, when setting up a link A->B.enabled=true (with defaults settings regarding heartbeat), it creates a topic heartbeat which produces beats on B
   
   I figured it was the other way around, so that the topic could be picked up by replication process, leading to a replicated topic B.heartbeat (which we monitor actually). 
   
   Looking into my configuration, I now understand that I was lucky because I have one link going up back up ``replica_OLS->replica_CENTRAL`` which is now the single emitter of beats (which are then replicated in every other cluster)
   
   Reading the KIP led me to interpret that the production of heartbeat would be done within the same herder (the source one)  : 
   > Internal Topics
   > MM2 emits a heartbeat topic in each source cluster, which is replicated to demonstrate connectivity through the connectors.
   
   Wouldn't it make more sense to produce the beats into the source side of the replication ?


----------------------------------------------------------------
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] aloknnikhil commented on a change in pull request #9589: KAFKA-10710 - Mirror Maker 2 - Create herders only if source->target.enabled=true

Posted by GitBox <gi...@apache.org>.
aloknnikhil commented on a change in pull request #9589:
URL: https://github.com/apache/kafka/pull/9589#discussion_r566536740



##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorMakerConfigTest.java
##########
@@ -256,6 +257,80 @@ public void testWorkerConfigs() {
             "secret2", bProps.get("producer.ssl.key.password"));
     }
 
+    @Test
+    public void testClusterPairsWithDefaultSettings() {
+        MirrorMakerConfig mirrorConfig = new MirrorMakerConfig(makeProps(
+                "clusters", "a, b, c"));
+        // implicit configuration associated
+        // a->b.enabled=false
+        // a->b.emit.heartbeat.enabled=true
+        // a->c.enabled=false
+        // a->c.emit.heartbeat.enabled=true
+        // b->a.enabled=false
+        // b->a.emit.heartbeat.enabled=true
+        // b->c.enabled=false
+        // b->c.emit.heartbeat.enabled=true
+        // c->a.enabled=false
+        // c->a.emit.heartbeat.enabled=true
+        // c->b.enabled=false
+        // c->b.emit.heartbeat.enabled=true
+        List<SourceAndTarget> clusterPairs = mirrorConfig.clusterPairs();
+        assertEquals("clusterPairs count should match all combinations count",

Review comment:
       @twobeeb Could you migrate these to use the new JUnit 5 `assertEquals` API? Looks like the build actually failed here - https://github.com/apache/kafka/runs/1782294502




----------------------------------------------------------------
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