[GitHub] [kafka] C0urante commented on a diff in pull request #13658: KAFKA-14669: Use the generated docs for MirrorMaker configs in the doc

2023-07-17 Thread via GitHub 


C0urante commented on code in PR #13658:
URL: https://github.com/apache/kafka/pull/13658#discussion_r1265805151


##
docs/ops.html:
##
@@ -766,27 +766,9 @@ https://github.com/apache/kafka/blob/trunk/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java;>MirrorMakerConfig
 and https://github.com/apache/kafka/blob/trunk/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorConnectorConfig.java;>MirrorConnectorConfig
 for further details.
+Additional configuration settings are supported which can be left with 
their default values in most cases. See https://kafka.apache.org/documentation/#mirrormakerconfigs;>MirrorMaker 
Configs.

Review Comment:
   Nit: we can just use a relative URL here
   ```suggestion
   Additional configuration settings are supported which can be left with 
their default values in most cases. See MirrorMaker Configs.
   ```



##
docs/configuration.html:
##
@@ -267,23 +267,38 @@ 
 
-  3.8 System 
Properties
+  3.8 MirrorMaker 
Configs
+  Below is the configuration of the connectors that make up MirrorMaker 2.
+  3.8.1 MirrorMaker 
Source Configs
+  Below is the configuration of MirrorMaker 2 source connector for replicating 
topics.
+  
+  3.8.2 
MirrorMaker Checkpoint Configs
+  Below is the configuration of MirrorMaker 2 checkpoint connector for 
emitting consumer offset checkpoints.
+  
+  3.8.3 
MirrorMaker HeartBeat Configs
+  Below is the configuration of MirrorMaker 2 heartbeat connector for checking 
connectivity between connectors and clusters.
+  
+  3.8.4 MirrorMaker 
Common Configs
+  Below is the common configuration properties that apply to all three 
connectors above.

Review Comment:
   Nit: plural
   ```suggestion
 Below are the common configuration properties that apply to all three 
connectors above.
   ```
   
   Also, IMO it makes more sense for the common properties to come first 
(someone may be expecting, e.g., `source.cluster.alias` to be included in the 
docs for a connector, and might be confused not to see it before scrolling 
further).



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

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] C0urante commented on a diff in pull request #13658: KAFKA-14669: Use the generated docs for MirrorMaker configs in the doc

2023-06-28 Thread via GitHub 


C0urante commented on code in PR #13658:
URL: https://github.com/apache/kafka/pull/13658#discussion_r1245804801


##
docs/configuration.html:
##
@@ -267,23 +267,31 @@ 
 
-  3.8 System 
Properties
+
+  3.8 MirrorMaker 
Configs
+  Below is the configuration of MirrorMaker.

Review Comment:
   Nit:
   ```suggestion
 Below is the configuration of the connectors that make up MirrorMaker 2.
   ```



##
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorHeartbeatConfig.java:
##
@@ -57,7 +59,7 @@ short heartbeatsTopicReplicationFactor() {
 return getShort(HEARTBEATS_TOPIC_REPLICATION_FACTOR);
 }
 
-protected static final ConfigDef CONNECTOR_CONFIG_DEF = new 
ConfigDef(BASE_CONNECTOR_CONFIG_DEF)
+protected static final ConfigDef HEARTBEAT_CONFIG_DEF = new ConfigDef()

Review Comment:
   I think we can get away without introducing a utility method to merge 
`ConfigDefs` by replacing this constant (and others like it) with a method that 
adds the connector-specific configuration properties to an existing `ConfigDef`:
   
   ```java
   private static ConfigDef defineHeartbeatConfig(ConfigDef baseConfig) {
   return baseConfig
   .define(
   EMIT_HEARTBEATS_ENABLED,
   ConfigDef.Type.BOOLEAN,
   EMIT_HEARTBEATS_ENABLED_DEFAULT,
   ConfigDef.Importance.LOW,
   EMIT_HEARTBEATS_ENABLED_DOC)
   // ...
   ```



##
docs/configuration.html:
##
@@ -267,23 +267,31 @@ 
 
-  3.8 System 
Properties
+
+  3.8 MirrorMaker 
Configs
+  Below is the configuration of MirrorMaker.
+  
+  

Review Comment:
   We should note the name of the connector here. It'd also be nice if we gave 
each section (common, source, checkpoint, heartbeat) a subheading, like we do 
for 
[source](https://github.com/apache/kafka/blob/3a246b1abab0cfa8050546f54c987af2ec6cdd4e/docs/configuration.html#L254C121-L254C145)
 and 
[sink](https://github.com/apache/kafka/blob/3a246b1abab0cfa8050546f54c987af2ec6cdd4e/docs/configuration.html#L258)
 connectors, and perhaps a brief (one sentence is fine) description of what 
each connector does and/or a link to other parts of our docs that already 
provide that info.



##
docs/configuration.html:
##
@@ -267,23 +267,31 @@ 
 
-  3.8 System 
Properties
+

Review Comment:
   Nit: unnecessary extra line
   ```suggestion
   ```



##
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorHeartbeatConfig.java:
##
@@ -77,7 +79,9 @@ short heartbeatsTopicReplicationFactor() {
 ConfigDef.Importance.LOW,
 HEARTBEATS_TOPIC_REPLICATION_FACTOR_DOC);
 
+protected final static ConfigDef CONNECTOR_CONFIG_DEF = new 
ConfigDef(mergeConnectorConfigDef(HEARTBEAT_CONFIG_DEF));

Review Comment:
   With the above suggestion, this can now become:
   ```java
   protected static final ConfigDef CONNECTOR_CONFIG_DEF = 
defineHeartbeatConfig(new ConfigDef(BASE_CONNECTOR_CONFIG_DEF));
   ```



##
docs/configuration.html:
##
@@ -267,23 +267,31 @@ 
 
-  3.8 System 
Properties
+
+  3.8 MirrorMaker 
Configs
+  Below is the configuration of MirrorMaker.
+  

Review Comment:
   We should note that these are common properties that apply to all three 
connectors, right?



##
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorHeartbeatConfig.java:
##
@@ -77,7 +79,9 @@ short heartbeatsTopicReplicationFactor() {
 ConfigDef.Importance.LOW,
 HEARTBEATS_TOPIC_REPLICATION_FACTOR_DOC);
 
+protected final static ConfigDef CONNECTOR_CONFIG_DEF = new 
ConfigDef(mergeConnectorConfigDef(HEARTBEAT_CONFIG_DEF));
+
 public static void main(String[] args) {
-System.out.println(CONNECTOR_CONFIG_DEF.toHtml(4, config -> 
"mirror_heartbeat_" + config));
+System.out.println(HEARTBEAT_CONFIG_DEF.toHtml(4, config -> 
"mirror_heartbeat_" + config));

Review Comment:
   With the above suggestion, this can now become:
   ```java
   System.out.println(defineHeartbeatConfig(new ConfigDef()).toHtml(4, 
config -> "mirror_heartbeat_" + config));
   ```



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

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org