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/07/30 07:30:06 UTC

[GitHub] [kafka] showuon opened a new pull request #9104: KAFKA-10266: Update the connector config header.converter

showuon opened a new pull request #9104:
URL: https://github.com/apache/kafka/pull/9104


   After my update, the original wrong statement will be removed.
   > By default, the SimpleHeaderConverter is used to ..... 
   
   And it'll be replaced with the following, with hyperlink to the worker config's header.converter section.
   >  By default, the value is null and the Connect config will be used.
   
   Also, update the default value to **Inherited from Connect config**
   
   ![圖片](https://user-images.githubusercontent.com/43372967/88890433-337b9d80-d274-11ea-8549-7c487faef823.png)
   
   
   ### 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] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine , could you help review this PR to correct the documentation. 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] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine , I've updated the PR, please review again. 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] kkonstantine commented on a change in pull request #9104: KAFKA-10266: Update the connector config header.converter

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



##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
##########
@@ -89,11 +89,16 @@
     public static final String VALUE_CONVERTER_CLASS_DISPLAY = "Value converter class";
 
     public static final String HEADER_CONVERTER_CLASS_CONFIG = WorkerConfig.HEADER_CONVERTER_CLASS_CONFIG;
-    public static final String HEADER_CONVERTER_CLASS_DOC = WorkerConfig.HEADER_CONVERTER_CLASS_DOC;
+    public static final String HEADER_CONVERTER_CLASS_DOC =
+            "HeaderConverter class used to convert between Kafka Connect format and the serialized form that is written to Kafka." +
+            " This controls the format of the header values in messages written to or read from Kafka, and since this is" +
+            " independent of connectors it allows any connector to work with any serialization format." +
+            " Examples of common formats include JSON and Avro. By default, the value will be inherited from" +
+            " the <a href=\"https://kafka.apache.org/documentation/#header.converter\">Connect config</a>.";
     public static final String HEADER_CONVERTER_CLASS_DISPLAY = "Header converter class";
     // The Connector config should not have a default for the header converter, since the absence of a config property means that
     // the worker config settings should be used. Thus, we set the default to null here.
-    public static final String HEADER_CONVERTER_CLASS_DEFAULT = null;
+    public static final String HEADER_CONVERTER_CLASS_DEFAULT = "Inherited from Connect config";

Review comment:
       What concerns me with this approach is that it can only be applied to String properties but not other property types. (The fact that this is not a valid default value is also a bit problematic). 
   
   Is there any other way to fix this?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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

[GitHub] [kafka] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine , please help review. 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] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine , could you check this PR? 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] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine , please help review this PR. 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] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine , could you review this PR? 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] showuon commented on pull request #9104: KAFKA-10266: Update the connector config header.converter

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


   @kkonstantine  , could you review this PR? 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] showuon commented on a change in pull request #9104: KAFKA-10266: Update the connector config header.converter

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



##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
##########
@@ -89,11 +89,16 @@
     public static final String VALUE_CONVERTER_CLASS_DISPLAY = "Value converter class";
 
     public static final String HEADER_CONVERTER_CLASS_CONFIG = WorkerConfig.HEADER_CONVERTER_CLASS_CONFIG;
-    public static final String HEADER_CONVERTER_CLASS_DOC = WorkerConfig.HEADER_CONVERTER_CLASS_DOC;
+    public static final String HEADER_CONVERTER_CLASS_DOC =
+            "HeaderConverter class used to convert between Kafka Connect format and the serialized form that is written to Kafka." +
+            " This controls the format of the header values in messages written to or read from Kafka, and since this is" +
+            " independent of connectors it allows any connector to work with any serialization format." +
+            " Examples of common formats include JSON and Avro. By default, the value will be inherited from" +
+            " the <a href=\"https://kafka.apache.org/documentation/#header.converter\">Connect config</a>.";
     public static final String HEADER_CONVERTER_CLASS_DISPLAY = "Header converter class";
     // The Connector config should not have a default for the header converter, since the absence of a config property means that
     // the worker config settings should be used. Thus, we set the default to null here.
-    public static final String HEADER_CONVERTER_CLASS_DEFAULT = null;
+    public static final String HEADER_CONVERTER_CLASS_DEFAULT = "Inherited from Connect config";

Review comment:
       @kkonstantine , thanks for the comments. I make the HEADER_CONVERTER_CLASS_DEFAULT as `WorkerConfig.HEADER_CONVERTER_CLASS_DEFAULT`, and I modified the description to make it more clear:
   > By default, the value will be inherited from the <a href=https://kafka.apache.org/documentation/#header.converter\>Connect config</a>. You can override it if needed.




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