[GitHub] [kafka] gharris1727 commented on a diff in pull request #13120: MINOR: Connect Javadocs improvements

["gharris1727 (via GitHub)" <gi...@apache.org>]

gharris1727 commented on code in PR #13120:
URL: https://github.com/apache/kafka/pull/13120#discussion_r1084469223


##########
connect/api/src/main/java/org/apache/kafka/connect/connector/policy/ConnectorClientConfigOverridePolicy.java:
##########
@@ -23,25 +23,25 @@
 import java.util.List;
 
 /**
- * <p>An interface for enforcing a policy on overriding of client configs via the connector configs.
- *
- * <p>Common use cases are ability to provide principal per connector, <code>sasl.jaas.config</code>
+ * An interface for enforcing a policy on overriding of Kafka client configs via the connector configs.
+ * <p>
+ * Common use cases are ability to provide principal per connector, <code>sasl.jaas.config</code>
  * and/or enforcing that the producer/consumer configurations for optimizations are within acceptable ranges.
  */
 public interface ConnectorClientConfigOverridePolicy extends Configurable, AutoCloseable {
 
 
     /**
-     * Worker will invoke this while constructing the producer for the SourceConnectors,  DLQ for SinkConnectors and the consumer for the
-     * SinkConnectors to validate if all of the overridden client configurations are allowed per the
-     * policy implementation. This would also be invoked during the validation of connector configs via the Rest API.
-     *
+     * Workers will invoke this while constructing producer for SourceConnectors, DLQs for SinkConnectors and
+     * consumers for SinkConnectors to validate if all of the overridden client configurations are allowed per the

Review Comment:
   We probably don't need to enumerate all of the use-cases for kafka clients here, and can keep that scoped to the ConnectorClientConfigRequest javadoc.
   ```suggestion
        * Workers will invoke this before configuring per-connector Kafka admin, producer, and consumer
        * client instances to validate if all of the overridden client configurations are allowed per the
   ```



##########
connect/api/src/main/java/org/apache/kafka/connect/connector/Connector.java:
##########
@@ -131,8 +131,8 @@ public void reconfigure(Map<String, String> props) {
     /**
      * Validate the connector configuration values against configuration definitions.
      * @param connectorConfigs the provided configuration values
-     * @return List of Config, each Config contains the updated configuration information given
-     * the current configuration values.
+     * @return {@link Config}, essentially a list of {@link ConfigValue}s containing the updated configuration
+     * information given the current configuration values.

Review Comment:
   ```suggestion
        * @return a parsed and validated {@link Config} containing any relevant validation errors with the raw
        * {@code connectorConfigs} which should prevent this configuration from being used.
   ```



##########
connect/api/src/main/java/org/apache/kafka/connect/sink/SinkTask.java:
##########
@@ -88,15 +88,15 @@ public void initialize(SinkTaskContext context) {
     public abstract void start(Map<String, String> props);
 
     /**
-     * Put the records in the sink. Usually this should send the records to the sink asynchronously
-     * and immediately return.
-     *
+     * Put the records in the sink. This should either write them to the downstream system or batch them for
+     * later writing

Review Comment:
   I agree that it's not necessary for this javadoc to prescribe asynchronous behavior, but it should certainly point out the pitfall that asynchronous users need to take special care.
   ```suggestion
        * Put the records in the sink. If this method returns before the records are durably written,
        * the task must implement {@link #flush(Map)} or {@link #preCommit(Map)} to ensure that
        * only durably written record offsets are committed, and that no records are dropped during failures.
   ```



##########
connect/api/src/main/java/org/apache/kafka/connect/connector/policy/ConnectorClientConfigRequest.java:
##########
@@ -44,25 +44,25 @@ public ConnectorClientConfigRequest(
     }
 
     /**
-     * Provides Config with prefix {@code producer.override.} for {@link ConnectorType#SOURCE}.
-     * Provides Config with prefix {@code consumer.override.} for {@link ConnectorType#SINK}.
-     * Provides Config with prefix {@code producer.override.} for {@link ConnectorType#SINK} for DLQ.
-     * Provides Config with prefix {@code admin.override.} for {@link ConnectorType#SINK} for DLQ.
+     * <p>Provides Config with prefix "{@code producer.override.}" for {@link ConnectorType#SOURCE}.
+     * <p>Provides Config with prefix "{@code consumer.override.}" for {@link ConnectorType#SINK}.
+     * <p>Provides Config with prefix "{@code producer.override.}" for {@link ConnectorType#SINK} for DLQ.
+     * <p>Provides Config with prefix "{@code admin.override.}" for {@link ConnectorType#SINK} for DLQ.

Review Comment:
   We should also mention some more recent usages of this that haven't been updated in this documentation:
   * Admin for fencing zombies in EOS source
   * Admin for creating connector-specific source offset topics
   * Consumer for connector-specific source offset topics
   * Producer for connector-specific source offset topics



-- 
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-unsubscribe@kafka.apache.org

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